Allow editing cell geometry definition#945
Allow editing cell geometry definition#945digvijay-y wants to merge 11 commits intoidaholab:developfrom
Conversation
…dd Test Cases Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds user-facing utilities to traverse and edit a cell’s CSG geometry tree, addressing #737 by enabling divider replacement and leaf iteration.
Changes:
- Added
HalfSpace.replace()/_replace_recursive()to swapSurface/Celldividers throughout a geometry tree. - Added
__iter__toHalfSpaceandUnitHalfSpacefor depth-first leaf traversal. - Added unit tests plus minor documentation/demo formatting updates.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
montepy/surfaces/half_space.py |
Implements replace() and iteration over geometry leaves; adjusts duplicate-surface remapping logic. |
tests/test_geometry.py |
Adds test coverage for replace() and __iter__. |
doc/source/changelog.rst |
Documents the new feature (but currently has RST formatting issues). |
doc/source/conf.py |
Removes trailing whitespace in nitpick_ignore. |
demo/_config.py |
Minor formatting (blank line). |
demo/1_fusion_radial_build.ipynb |
Minor formatting fix (newline). |
demo/answers/1_fusion_radial_build.ipynb |
Formatting cleanup in example code cells. |
| Next Release | ||
| ============ | ||
|
|
||
| **Features Added** |
There was a problem hiding this comment.
The new "Next Release" section has a bullet list immediately following **Features Added** without a blank line. reStructuredText generally requires a blank line before lists; without it, Sphinx/docutils can emit warnings (and CI treats warnings as errors). Add a blank line between the bold heading and the * list to avoid doc build failures.
| **Features Added** | |
| **Features Added** |
| replaced = self._replace_recursive(old_divider, new_divider) | ||
| if not replaced: | ||
| raise ValueError( | ||
| f"{old_divider} (number: {old_divider.number}) not found in geometry tree." | ||
| ) | ||
| # _cell is only set on UnitHalfSpace leaves, not on internal HalfSpace nodes. | ||
| # Find the parent cell via any leaf and remove the old divider from its collection. | ||
| for leaf in self: | ||
| if leaf._cell is not None: | ||
| container = ( | ||
| leaf._cell.complements | ||
| if isinstance(old_divider, montepy.Cell) | ||
| else leaf._cell.surfaces | ||
| ) | ||
| if old_divider in container: | ||
| container.remove(old_divider) | ||
| break |
There was a problem hiding this comment.
HalfSpace.replace() will remove old_divider from the parent cell container even when old_divider is new_divider (a no-op replacement). In that case the geometry still references the divider but it gets removed from cell.surfaces/cell.complements, leaving the cell’s containers inconsistent. Add an early-return when old_divider is new_divider, or at least skip the container cleanup in that case.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| def replace( | ||
| self, | ||
| old_divider: montepy.surfaces.surface.Surface | montepy.Cell, | ||
| new_divider: montepy.surfaces.surface.Surface | montepy.Cell, | ||
| ) -> None: |
There was a problem hiding this comment.
replace() is documented and typed to support swapping either Surface dividers or Cell complements, but the added tests cover only surface replacement. Add at least one test that replaces a complemented Cell divider and asserts both the geometry leaves and cell.complements are updated correctly (including removal of the old cell).
| replaced = self._replace_recursive(old_divider, new_divider) | ||
| if not replaced: | ||
| raise ValueError( | ||
| f"{old_divider} (number: {old_divider.number}) not found in geometry tree." | ||
| ) |
There was a problem hiding this comment.
replace() can be called with the same object for both arguments (old_divider is new_divider). In that case _replace_recursive() will report success, but the cleanup loop later will still remove old_divider from the parent cell’s surfaces/complements, leaving the geometry referencing a divider that is no longer in the cell collection. Add an early return/no-op when old_divider is new_divider (or skip container removal in that case).
| if not isinstance( | ||
| old_divider, (montepy.surfaces.surface.Surface, montepy.Cell) | ||
| ): | ||
| raise TypeError( | ||
| f"old_divider must be a Surface or Cell. {old_divider} given." | ||
| ) | ||
| if not isinstance( | ||
| new_divider, (montepy.surfaces.surface.Surface, montepy.Cell) | ||
| ): | ||
| raise TypeError( | ||
| f"new_divider must be a Surface or Cell. {new_divider} given." | ||
| ) |
There was a problem hiding this comment.
replace() currently assumes the geometry tree has been linked via update_pointers() (i.e., leaf dividers are Surface/Cell objects). If the tree still contains integer dividers from parsing, _replace_recursive() will fail to match and this raises a misleading ValueError (“not found”). Consider explicitly validating the tree is initialized (e.g., by calling _get_leaf_objects() up front, or checking for Integral dividers) and raising IllegalState with the existing “Run Cell.update_pointers” guidance instead.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@digvijay-y thanks for doing this. It will be a few days before I will be able to review this. |
Pull Request Checklist for MontePy
Description
Fixes #737
General Checklist
blackversion 25 or 26.LLM Disclosure
Are you?
Were any large language models (LLM or "AI") used in to generate any of this code?
Documentation Checklist
First-Time Contributor Checklist
pyproject.tomlif you wish to do so.Additional Notes for Reviewers
Ensure that:
📚 Documentation preview 📚: https://montepy--945.org.readthedocs.build/en/945/