Allow editing cell geometry definition by digvijay-y · Pull Request #945 · idaholab/MontePy

digvijay-y · 2026-04-13T18:22:13Z

Pull Request Checklist for MontePy

Description

Add iter to HalfSpace and unit Halfspace
Add replace() to HalfSpace
Add _replace_recursive() to both classes
Add TestCases

Fixes #737

General Checklist

I have performed a self-review of my own code.
The code follows the standards outlined in the development documentation.
I have formatted my code with black version 25 or 26.
I have added tests that prove my fix is effective or that my feature works (if applicable).

LLM Disclosure

Are you?
- A human user
- A large language model (LLM), including ones acting on behalf of a human
Were any large language models (LLM or "AI") used in to generate any of this code?

Yes
- Model(s) used: Claude sonnet 4.5 (anthropic) - Built Test cases.
No

Documentation Checklist

I have documented all added classes and methods.
For infrastructure updates, I have updated the developer's guide.
For significant new features, I have added a section to the getting started guide.

First-Time Contributor Checklist

If this is your first contribution, add yourself to pyproject.toml if you wish to do so.

Additional Notes for Reviewers

Ensure that:

This PR fully addresses and resolves the referenced issue(s).
The submitted code is consistent with the merge checklist outlined here.
The PR covers all relevant aspects according to the development guidelines.
100% coverage of the patch is achieved, or justification for a variance is given.

📚 Documentation preview 📚: https://montepy--945.org.readthedocs.build/en/945/

…dd Test Cases Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>

Copilot

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 swap Surface/Cell dividers throughout a geometry tree.
Added __iter__ to HalfSpace and UnitHalfSpace for 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.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Copilot · 2026-04-26T04:04:13Z

+    def replace(
+        self,
+        old_divider: montepy.surfaces.surface.Surface | montepy.Cell,
+        new_divider: montepy.surfaces.surface.Surface | montepy.Cell,
+    ) -> None:


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).

Copilot · 2026-04-26T04:04:14Z

+        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."
+            )


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).

Let's just add a guard like:

if new_divider is old_divider: raise ValueError(...)

Copilot · 2026-04-26T04:04:14Z

+        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."
+            )


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.

I would suggest using the Integral check and raising IllegalState error. This is quite an edge case that I don't think a ton of effort needs to be put into resolving it (e.g., _get_leaf_objects)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

MicahGale · 2026-04-27T19:41:41Z

@digvijay-y thanks for doing this. It will be a few days before I will be able to review this.

Copilot

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

MicahGale

Thanks again @digvijay-y.

A few thoughts:

This is a good first implementation, just some slight improvements can be made.
I messed up the GHA so coveralls doesn't run. There is some uncovered code. Could you check if these were your changes?

montepy/surfaces/half_space.py                  366      4  98.91%   208, 429, 473, 484

Could you review the self-check of your code?

MicahGale · 2026-05-08T22:00:18Z

-    "    184: 0.306,\n",
-    "    186: 0.284\n",
-    "}\n",
+    "w_natural = {182: 0.265, 183: 0.143, 184: 0.306, 186: 0.284}\n",


Could you revert this? I don't like how black formatted this, and demo is exempt from formatting.

MicahGale · 2026-05-08T22:01:40Z

+    def replace(
+        self,
+        old_divider: montepy.surfaces.surface.Surface | montepy.Cell,
+        new_divider: montepy.surfaces.surface.Surface | montepy.Cell,
+    ) -> None:


MicahGale · 2026-05-08T22:02:14Z

+        old_divider: montepy.surfaces.surface.Surface | montepy.Cell,
+        new_divider: montepy.surfaces.surface.Surface | montepy.Cell,


the type montepy.Surface should work as well and would be clearer.

MicahGale · 2026-05-08T22:06:39Z

+        old_divider : Surface, Cell
+            the divider to be replaced.
+        new_divider : Surface, Cell


Suggested change

old_divider : Surface, Cell

the divider to be replaced.

new_divider : Surface, Cell

old_divider : Surface or Cell

the divider to be replaced.

new_divider : Surface or Cell

The numpy standard is to use or.

MicahGale · 2026-05-08T22:09:18Z

+        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."
+            )


I would suggest using the Integral check and raising IllegalState error. This is quite an edge case that I don't think a ton of effort needs to be put into resolving it (e.g., _get_leaf_objects)

MicahGale · 2026-05-08T22:20:00Z

+def _make_linked_geometry(*surfs):
+    """Helper: build a parent cell whose geometry is already linked.
+
+    Returns (parent_cell, half_space) where every UnitHalfSpace leaf has
+    self._cell set so the old-divider cleanup path in replace() is exercised.
+    """
+    parent = montepy.Cell()
+    parent.number = 99
+    # Populate parent.surfaces up front so all surfaces are present before replace()
+    for surf in surfs:
+        parent.surfaces.append(surf)
+    half_space = None
+    for surf in surfs:
+        leaf = +surf
+        # Wire _cell directly since dividers are already resolved objects, not ints
+        leaf._cell = parent
+        half_space = leaf if half_space is None else half_space & leaf
+    return parent, half_space


These tests probably live better in tests/test_geometry_integration.py. I think you will have fixtures there you can use for this. At the very least the cell should be coming from a @pytest.fixture I think.

MicahGale · 2026-05-08T22:22:50Z

    def __len__(self):
        return 1

+    def __iter__(self):


I like this addition; surprised it hadn't existed before.

MicahGale · 2026-05-08T22:23:45Z

+
+def test_replace_surface_basic():
+    """replace() swaps old surface for new one throughout the tree."""
+    surf1 = montepy.CylinderOnAxis()


just use montepy.CylinderOnAxis(number=1) to be more concise.

MicahGale · 2026-05-08T22:26:09Z

+    surf2.number = 2
+    surf3.number = 3
+    half_space = +surf1 & -surf2 & +surf3
+    assert len(list(half_space)) == len(half_space)


This is calling __iter__ twice. Should just do len(half_space) == 3

MicahGale · 2026-05-08T22:27:20Z

+    dividers = [leaf.divider for leaf in leaves]
+    assert surf3 in dividers
+    assert surf1 not in dividers
+    assert surf2 in dividers


We should also verify that the geometry is written to file correctly. I think test_geometry_integration.py has a function for doing this verify_export or something along those lines.

digvijay-y added 5 commits April 13, 2026 18:16

__iter__ for halfspace and unit

4cb2740

Merge branch 'develop' into geometry-editing

4c397d2

Add replace and Replace_recursive func

602abf4

updated Changelog

ea99b45

Merge branch 'develop' into geometry-editing

96e6617

digvijay-y added the feature request An issue that improves the user interface. label Apr 25, 2026

digvijay-y added 2 commits April 25, 2026 10:51

Enhance replace() method in HalfSpace and UnitHalfSpace classes and A…

1f4d142

…dd Test Cases Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>

Fix half_space.py for testcase failures

9706b96

digvijay-y marked this pull request as ready for review April 26, 2026 03:53

Copilot AI review requested due to automatic review settings April 26, 2026 03:53

Copilot started reviewing on behalf of digvijay-y April 26, 2026 03:53 View session

digvijay-y self-assigned this Apr 26, 2026

Copilot AI reviewed Apr 26, 2026

View reviewed changes

Comment thread montepy/surfaces/half_space.py Outdated

Comment thread doc/source/changelog.rst

Comment thread doc/source/changelog.rst

Comment thread montepy/surfaces/half_space.py

digvijay-y and others added 2 commits April 26, 2026 03:59

Fix formatting of changelog entry for HalfSpace.replace method

a31da76

Update doc/source/changelog.rst

ad40ae1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 26, 2026 04:00

Update montepy/surfaces/half_space.py

e0088d6

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot started reviewing on behalf of digvijay-y April 26, 2026 04:01 View session

Copilot AI reviewed Apr 26, 2026

View reviewed changes

Update doc/source/changelog.rst

9e89c37

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 26, 2026 04:05

Copilot started reviewing on behalf of digvijay-y April 26, 2026 04:05 View session

digvijay-y requested review from MicahGale and removed request for Copilot April 26, 2026 04:06

Merge branch 'develop' into geometry-editing

0d04d57

Copilot AI review requested due to automatic review settings May 8, 2026 21:59

Copilot AI reviewed May 8, 2026

View reviewed changes

MicahGale requested changes May 8, 2026

View reviewed changes

		old_divider: montepy.surfaces.surface.Surface \| montepy.Cell,
		new_divider: montepy.surfaces.surface.Surface \| montepy.Cell,

Conversation

digvijay-y commented Apr 13, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist for MontePy

Description

General Checklist

LLM Disclosure

Documentation Checklist

First-Time Contributor Checklist

Additional Notes for Reviewers

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MicahGale commented Apr 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

MicahGale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

digvijay-y commented Apr 13, 2026 •

edited by github-actions Bot

Loading