Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged

Conversation

shimwell
Copy link
Collaborator

@shimwell shimwell commented Jun 1, 2024

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:

  • The CLI example JSON files in the docs have been updated
  • The example API usage in the docs have been updated
  • The new release will have release notes that identify this change
  • The next release should be a minor version increase which helps hint at breaking changes
  • The current version 1.1.0 remains installable with with conda install -c conda-forge geouned==1.1.0 so users can always install an older version if they want.
  • The docs for the older versions also remain accessable via the docs version dropdown menu
  • users with an IDE will get autocomplete to indeicate that Settings no longer has the cellRange attribute
  • standard python error messages also exist like error Settings does not have attribute cellRange

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

#204

Checklist

  • I'm making a PR from a feature branch on my fork into GEOUNED-org/GEOUNED/dev branch
  • I have followed PEP8 style guide for Python or run a formatter such as black or ruff format on my code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@shimwell
Copy link
Collaborator Author

shimwell commented Jun 3, 2024

this one is read for review if anyone has time

@psauvan psauvan merged commit 1762a3a into GEOUNED-org:dev Jun 4, 2024
9 checks passed
psauvan pushed a commit that referenced this pull request Jun 17, 2024
… loading geometry (#205)

* moved two methods out of start

* removed unnecessary logger line

* changed cellRange to skip_solids

* improved doc string

* lowercase args pep8
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants