Uh oh!
There was an error while loading. Please reload this page.
Compute bounding boxes for general planes and tori in C++ - #4087
Open
dylanpulver wants to merge 1 commit into
Open
Compute bounding boxes for general planes and tori in C++#4087dylanpulver wants to merge 1 commit into
dylanpulver wants to merge 1 commit into
Conversation
The C++ Surface::bounding_box overrides cover the axis-aligned planes, the axis-aligned cylinders and the sphere, but not SurfacePlane or the three torus classes, while the Python API computes a box for all four. A cell bounded by a general plane that is axis-aligned to within roundoff therefore reports a different bounding box through openmc.lib.Cell.bounding_box than through openmc.Cell.bounding_box. Add the four missing overrides, mirroring the Python implementations. PlaneMixin.bounding_box tested for axis alignment using the normalized normal but chose per-axis intercepts using the raw coefficients. Those criteria disagree for off-axis coefficients between the tolerance and roughly 1e-6, where the resulting box excluded points that lie inside the half-space. Apply the alignment tolerance consistently so both implementations agree and neither produces such a box.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
SurfacePlaneand the three torus classes have nobounding_boxoverride, so they fall through toSurface::bounding_boxand return an infinite box, while the Python API computes a real one for all four. That is the inconsistency in #2632:s64there is a y-plane written withcos(pi/2)in the x and z slots, whichPlaneMixinrecognizes as axis-aligned and C++ does not.This adds the four overrides, following @paulromano's suggestion on the issue to give
SurfacePlanethe logicPlaneMixin.bounding_boxalready has. The tori are included because they have the same gap, and there C++ was returning an infinite box for a region that is actually compact.One thing I did not expect, and the only judgement call here.
PlaneMixin.bounding_boxdecided alignment from the normalized normal but picked intercepts from the raw coefficients. Those disagree when an off-axis coefficient is above_atolwhile the normal is still within it — roughly 1e-12 to 1e-6, moving with the scale of the coefficients since the raw test is not scale-invariant. In that band the box excluded points inside its own half-space:Plane(1, 1e-11, 0, 5)gavell = [5, 5e11, -inf]. Mirroring that into C++ would have shipped a box that makes volume calculations miss volume, so the tolerance is now applied consistently on both sides. Happy to split that into its own PR if you would rather look at it separately.Verification. Differential harness, 224 surfaces x both senses, Python vs C++ through
openmc.lib's ownDBL_MAX→infconvention: 116 divergences before, 0 after, with every already-implemented surface type agreeing on both sides as a built-in control. An independent oracle that reads neither implementation — check that any point satisfying the half-space inequality lies inside the reported box — went from 35,213 violations to 0 over 160,000 in-half-space points.New
tests/cpp_unit_tests/test_surface.cpp, 66 assertions, including the cell from the issue asserted against the values reported there. Mutants: reverting the source kills 27 assertions, a literal port of the Python logic kills 7, dropping the alignment gate kills 12. 38 of 66 discriminate; the other 28 guard directions that must stay unbounded.Scope. macOS/arm64, no MPI/DAGMC/libMesh. C++ unit tests 12/13,
test_mcpl_stat_sumfailing identically before and after because MCPL is not installed here. Pythontest_surface,test_bounding_box,test_surface_compositeandtest_geometry: 89 passed / 1 failed identically on both sides, that failure pre-existing and wanting anopenmcexecutable on PATH.test_region::test_plotandtest_universeabort the interpreter here, also identically on both sides. The regression suites need cross-section data this machine does not have and were not run.clang-format18 clean.Fixes#2632
Disclosure: the code, the tests and this description were drafted by Claude Opus 5 working as a coding agent in this repository.
Checklist