Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that implicit complement cells appear last in DAGMC universes #2838

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

paulromano
Copy link
Contributor

@paulromano paulromano commented Jan 12, 2024

Description

The bug that this fixes is described in lots of detail at svalinn/DAGMC#934. It's debatable whether this is a DAGMC or OpenMC bug, but in either case, this PR will fix the behavior. Namely, for some DAGMC models, using MOAB 5.4+ currently results in particles incorrectly being found in the implicit complement volume when they shouldn't be. The change here ensures that all other volumes are checked first.

@pshriwise I don't have a test for this because I can't think of how one would do it. Let me know if you have any ideas.

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@pshriwise
Copy link
Contributor

pshriwise commented Jan 12, 2024

Thanks for tackling this @paulromano!! 💚

The shift in the EntityHandle sequence is really something we should test for in DAGMC, so I wouldn't worry about testing that.

Here in OpenMC, we could create a C++ unit test that loads one of the DAGMC files, creates a DAGUniverse object, and then verifies that the implicit complement index is at the back of the DAGUniverse::cells_ attribute.

@paulromano
Copy link
Contributor Author

In addition to fixing the underprediction in keff that is discussed in svalinn/DAGMC#934, I'm happy to report that this also eliminates a lot of plotting artifacts that I (and others) have seen when using MOAB 5.4+. For example, here is a sample plot from the MSRE CAD model:

Before this PR
Screenshot from 2024-01-12 13-48-59

After this PR
Screenshot from 2024-01-12 13-38-34

@shimwell
Copy link
Member

shimwell commented Jan 15, 2024

Confirming this makes nicer plots of the openmsr
Before
yz_material
After
yz_material2

@paulromano
Copy link
Contributor Author

@pshriwise I just updated one of our existing DAGMC tests to check for the implicit complement cell ordering.

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you @paulromano.

@pshriwise pshriwise enabled auto-merge (squash) January 15, 2024 19:27
@pshriwise pshriwise merged commit 971c5f7 into openmc-dev:develop Jan 15, 2024
17 checks passed
@paulromano paulromano deleted the implicit-complement-ordering branch January 15, 2024 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants