DAGMC bounding surface IDs
@shimwell recently noted that while the bounding_cell_id argument of DAGMCUniverse.bounded_universe defaults to a high cell ID to avoid clashing with the ID space of the DAGMC model, the surface IDs are still likely to clash. This PR addresses that issue and adds a couple of additional checks for both the cell and surface IDs in the tests.
Thanks for this fix @pshriwise I have repoduced the surface id error and then switched the CI to use your fork and can confirm it fixed the bug.
I documented some testing over here on this PR https://github.com/shimwell/openmc_diff/pull/7
Looks good to me, passes the CI on the openmc_diff repo ✔️ . Many thanks
I've added some unit tests that check for DAGMCUniverse.bounded_universe, DAGMCUniverse.bounding_region and DAGMCUniverse.bounding_box properties to a PR on your fork for consideration. No worries if they are not useful
https://github.com/pshriwise/openmc/pull/14
Thank you for the PR @shimwell! These checks are much better off under unit_tests. I've tweaked them a little bit after merging your PR. Would you mind taking at those tests one more time?
Thanks for updating the tests and allowing the filename to be a Path , very nice.
I've tested this over on my openmc_diff CI :heavy_check_mark: as well as reviewing the changes here.
I've run those new unit tests locally and they also pass :heavy_check_mark:, (and they didn't pass with the develop branch)
We can wait 36h for others to comment. If there are no other comments then I can merge this in on Sunday