GEOUNED-org / GEOUNED

A tool to convert CAD to CSG & CSG to CAD for Monte Carlo transport codes
European Union Public License 1.2
45 stars 27 forks source link

Changing cellRange to discrete numbers to allow more flexability when loading geometry #205

Closed shimwell closed 3 weeks ago

shimwell commented 4 weeks ago

Description

This PR removes the Settings.cellRange and replaces it with a more flexible skip_solids argument directly on the load_step_file method. The current Settings.cellRange only allows for a continious range of cells to be selected (e.g. cell 10 to 12 which would be cell 10,11,12. This is not very useful if a user wants cell 10, 12 and 14 which can't be done with Settings.cellRange as it is not a continuous range. skip_solids however allows both types of filtering.

This it easier for users to find this functionally as it is entered into the ``load_step_file``` function where one would expect to set solid filtering when loading a step file. Previously it was hidden away in the Settings class and users would have little chance to know that it exists.

Additionally the the cellRange setting was only used in the loading of the cad geometry and was being passed around most of the code in the Settings object unnecessarily. Now it has been removed from the settings object it is not longer passed around the code unnecessarily.

The Settings.cellRange was never tested in the pytests or CI. skip_solids is now tested in the pytest suite under CI so we can be more confident that it works.

The other change this PR makes is that the python API usage now needs users to perform CsgToCad.load_step_file(filename='example.stp') as a step in the process. Previously it was part of the large start() method. Now it has a similar usage to the export_csg method as users call it directly.

This looks better that the previous specification of stepFile on the class instance, which also meant it had its own key in the JSON config file format. This meant the JSON was not consistent as all the other keys are class or method names.

This is a small breaking change to the API but I think it is worth the minor disruption. This is mitigated by:

Although I don't think anyone used the cellRange as it was hard to find and only able to skip adjected solid IDs. skip_solids is more flexible as it can skip an arbitrary list of solid IDs, it is easier to find as it is located as an arg to the only method that uses it and it is tested.

Just a note the cellRange / skip_solids when loading the cad file is not actually doing what people might assume. Both methods loads the whole cad file and then retrospectively deletes solids from a list. Naturally this doesn't save any loading time. I think this PR is still an improvement on the cell Ranges approach but I think there are also better ways of doing this filtering of cells later in the process. I would personally not do this skipping on loading the cad as it makes skipping solids by their original ID number in later steps not possible. Really we want to offer skipping solids on later steps like decompose or export or convert steps. But as the original index of the solids is lmchanged by the cellRange/skip solids on when loading the cad file we can't offer solid cell filtering later in the process as the index numbers would be wrong.

Fixes issue

https://github.com/GEOUNED-org/GEOUNED/issues/204

Checklist

shimwell commented 3 weeks ago

this one is read for review if anyone has time