openmc icon indicating copy to clipboard operation
openmc copied to clipboard

DAGMC bounding surface IDs

Open pshriwise opened this issue 3 years ago • 5 comments

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

pshriwise avatar Aug 11 '22 02:08 pshriwise

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

shimwell avatar Aug 11 '22 08:08 shimwell

Looks good to me, passes the CI on the openmc_diff repo ✔️ . Many thanks

shimwell avatar Aug 11 '22 13:08 shimwell

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

shimwell avatar Aug 11 '22 17:08 shimwell

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?

pshriwise avatar Aug 12 '22 12:08 pshriwise

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

shimwell avatar Aug 12 '22 14:08 shimwell