Skip to content

Add bounds calculation for geometric sets. - #59

Merged
gonuke merged 2 commits into
mainfrom
bounds
Dec 3, 2025
Merged

Add bounds calculation for geometric sets.#59
gonuke merged 2 commits into
mainfrom
bounds

Conversation

@pshriwise

Copy link
Copy Markdown
Member

This adds the ability to return a minimum and maximum coordinate for GeometricSet via a bounds property. This is useful for defining source boundaries in downstream codes from Python.

@gonuke gonuke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this passes tests as expected, but I'm not sure the data structure this returns is as obvious as the comments suggest... maybe I'm just a little confused?

Comment thread src/pydagmc/dagnav.py
coords = self.triangle_coords
min_coord = np.min(coords, axis=0)
max_coord = np.max(coords, axis=0)
return min_coord, max_coord

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this (xmin, ymin, zmin), (xmax, ymax, zmax)? Is it really a tuple? or is it a pair of numpy arrays that can be iterated like a tuple like they are in the tests?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad here. I changed the data structure to something I liked better but didn't update the docs/expected test data structure -- the result of pytest.approx was a boolean array that it always considered to be true. No bueno. I brought in numpy.testing.assert_equal to remedy this. I also selected different surface bounds. Some which are the top and bottom of the cylinder and have the same min/max coordinate in one dimension for more varitey.

@gonuke gonuke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes a little more sense now 😀 - thanks @pshriwise

@gonuke
gonuke merged commit 68dad2e into main Dec 3, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants