[BUG] - conversions inherit previous conversion attributes

[shimwell]

Describe the bug

When running more than one conversion in the same python script the attributes of Options, Tolerances and Numerical_format are carried between conversions.

This is difficult for the user to spot and could lead to a decent amount of confusion.

For example if the first conversion changes any of these setting in their config.ini file and does not over write them with the next config file then these new settings be carried between conversions. Impacts this settings in the config

[Options]
forceCylinder
newSplitPlane
delLastNumber
verbose
enlargeBox
nPlaneReverse
splitTolerance
scaleUp
quadricPY
Facets
[Tolerances]
prnt3PPlane
relativeTol
relativePrecision
value
distance
angle
pln_distance
pln_angle
cyl_distance
cyl_angle
sph_distance
kne_distance
kne_angle
tor_distance
tor_angle
min_area
[MCNP_numeric_format]
P_abc
P_d
P_xyz
S_r
S_xyz
C_r
C_xyz
K_xyz
K_tan2
T_r
T_xyz
GQ_1to6
GQ_7to9
GQ_10

To Reproduce

this example converts a stp file with a set relativePrecision then converts a second step file without specifying the relativePrecision and it would end up not having the default Tolerance.relativePrecision but instead it would get the relativePrecision setting from the previous conversion

from pathlib import Path
from geouned import GEOUNED
from geouned import Tolerances

print(Tolerances.relativePrecision)
#  prints to terminal the default value of relativePrecision
#  which is set in here in the code https://github.com/GEOUNED-org/GEOUNED/blob/bc74e7ef28a50290fdfd3847cea7c1956fea3418/src/geouned/GEOUNED/Utils/Options/__init__.py#L18
# >>> 1e-06

# finds a cad file to convert
path_to_cad = Path("testing/inputSTEP")
step_files = list(path_to_cad.rglob("*.stp")) + list(path_to_cad.rglob("*.step"))
input_step_file = step_files[2]


# creates the config file contents
template = (
    "[Files]\n"
    "title = 'Input Test'\n"
    f"stepFile = {input_step_file}\n"
    f"geometryName = '1st_conversion\n"
    "outFormat = ('mcnp', 'openMC_XML')\n"
    "[Parameters]\n"
    "compSolids = False\n"
    "volCARD = False\n"
    "volSDEF = True\n"
    "voidGen = True\n"
    "dummyMat = True\n"
    "minVoidSize = 100\n"
    "cellSummaryFile = False\n"
    "cellCommentFile = False\n"
    "debug = False\n"
    "simplify = no\n"
    "[Options]\n"
    "forceCylinder = False\n"
    "splitTolerance = 0\n"
    "newSplitPlane = True\n"
    "nPlaneReverse = 0\n"
    "[Tolerances]\n"
    "relativePrecision = 2e-6"  # here we make u new number for relativePrecision
)

with open("config1.ini", mode="w") as file:
    file.write(template)

GEO = GEOUNED('config1.ini')
GEO.SetOptions()  # this changes relativePrecision to our new number 2e-6
GEO.Start()

# Tolerances stays set as the new number
print(f'Tolerances.relativePrecision= {Tolerances.relativePrecision}')

# now if we want to do another conversion
# this time we don't state the tolerances in the config
template = (
    "[Files]\n"
    "title = 'Input Test'\n"
    f"stepFile = {input_step_file}\n"
    f"geometryName = '2nd_conversion'\n"
    "outFormat = ('mcnp', 'openMC_XML')\n"
    "[Parameters]\n"
    "compSolids = False\n"
    "volCARD = False\n"
    "volSDEF = True\n"
    "voidGen = True\n"
    "dummyMat = True\n"
    "minVoidSize = 100\n"
    "cellSummaryFile = False\n"
    "cellCommentFile = False\n"
    "debug = False\n"
    "simplify = no\n"
    "[Options]\n"
    "forceCylinder = False\n"
    "splitTolerance = 0\n"
    "newSplitPlane = True\n"
    "nPlaneReverse = 0\n"
)

with open("config2.ini", mode="w") as file:
    file.write(template)

GEO = GEOUNED('config2.ini')
GEO.SetOptions()  # this does not change the relativePrecision back to the default
GEO.Start()

# Tolerances stays set as the new number and the 2nd conversion uses the tolerance of the first example
print(f'Tolerances.relativePrecision= {Tolerances.relativePrecision}')

Expected behavior

When the user makes a new GEOUNED instance then it should come with the defaults, not with the previous conversion values.

Error message

no error message

Please complete the following information):

• OS: linuix
• GEOUNED dev
• Python version 3.10

[psauvan]

This is because "Options" and "Tolerances" parameters are stored in a class as class attributes. During the initialization the set method changes the attribute value of the classes "Options" and "Tolerances" for the attribute name defined in the user template. If in the second call of the GEOUNED instance the attributes changed in the first instance call are not defined as input data in the second template, their value doesn't change from the first assignation because, the classes "Options" or "Tolerances" are the same as in the first call of the GEOUNED instance (after updating the value of their attibute).
The solution would be to create a new "Options" and "Tolerances" instances each time the GEOUNED instance is called or delete these classes if there exits when GEOUNED instance is called.
P.D.: I have defined the parameters of the options and tolerance through classes and class attibutes to have access to their values in the code as global variables (I don't know if it is the best way to do this)

[shimwell]

Thanks for the update

Getting geouned to internaly make new tolerances and options instances to reset their values to defaults sounds most reasonable to me. Then the users are guaranteed to get the defaults that are mentioned in the docs.

We could look into refactoring and perhaps remove the need for a global in the longer term

[shimwell]

I think a decompose strategy would be a natural solution here. It feels a bit excessive to expose all 63 attributes to the user in a single class interface.

Users who want to specify everything could make make 3 classes and bring them together for the conversion.

We currently have:

• 14 attributes for tolerances
• 12 for options
• 13 attributes for MCNP_numerical_format
• 24 attributes for the main geouned class (now called CadToCsg)

Here is how user level code could look

I've used the same variable names as we currently have in the code but I am currently trying to snake_case variables and args and CamelCase classes so these may change. However here is a general layout for discussion

from geouned import CadToCsG, Options, Tolerances, MCNP_numeric_format

my_options = Options(
    forceCylinder=True,
    forceNoOverlap=True,
)

my_tolerances = Tolerances(
    relativeTolerance=1e-6,
    relativePrecision=2.7e1
)

my_num_format = MCNP_numeric_format(
    P_xyz="14.7e",
    GQ_1to6": "18.15"
)

my_conversion = CadToCsg(
    voidGen=True,
    compSolids =False,
    volSDEF =True,
    startCell = 20,
    options=my_options ,
    tolerances=my_tolerances,
    number_format=my_num_format
)
my_conversion.start()

The arguments might want to be moved around a bit and one thing we could consider is to provide separate export methods on the CadToCsg which could remove some of the MC code specific attributes from the init.

For example we could have class methods like CadToCsg.export_mcnp(startCell=2) which would move some of the CadToCsg attributes to method arguments. This particular method argument would also mean we need to perform and 'cache' a conversion then allow output to different codes.

Naturally we must also maintain compatibility with the config.ini input file method. I think we could have a CadToCsg.from_config(filename='config.ini') classmethod which would basically read the config and Instantiate the CadToCsg class with the correct attributes, options, tolerances and numerical_format```

keen to hear other thoughts

[shimwell]

If people are happy with this plan then the first course of action would be to move away from having Options, Tolerances, MCNP_numeric_format available on the global scope of the whole project.

Instead pass them from CadToCsg to the methods that need information from these classes

[psauvan]

The proposed PR #102 is doing more or less what your are proposing.

Originally I separated the input parameters in four groups (as they are now) depending on the type of parameters and their use for the translation process.
The first group which is the one correspond to attributes passed directly to the class ("stepFile","outFormat",....) are the basic inputs that can change on each different run (same model to translate or different one).
The other groups defined as classes are not supposed to be changed from one simulation to other, they are here only if in specific case the translation is not good right and intermediate/advanced user want to change some parameter to solve the issue. The parameters passed through the class may have been hard coded,

Returning to your proposal, I am agree to pass the Options, Tolerances, and McnpNumericFormat as local classes to each CadToCSG instance. But formerly, when I tryied to do this, I face the problem that there were several method calls between the point where the class were defined and the methods/functions using their attributes, and passing the information through the different calls wasn't easy nor elegant. That's why I decided to use global classes.

I think this work should be included as part of the refactoring work, and cannot be performed easily as isolated task.

[psauvan]

I didn't understand what method like CadToCsg.export_mcnp(startCell=2) pretend to do.

[shimwell]

I didn't understand what method like CadToCsg.export_mcnp(startCell=2) pretend to do.

This would export/write the input file for mcnp with a options set for the startcell.

Currently the Options class has some attributes such as quadricPY which I believe are only used for one MC code. So if there are any attributes that we can not add to the class and instead delay their definition to a yet to exist export function then this would reduce the number of class attributes. For examplke with quadricPY I think this is an openmc only attribute so MCNP users don't need to see this in their workflow

[shimwell]

solved by #124