-
Notifications
You must be signed in to change notification settings - Fork 582
Reset DAGMC history when reviving from source. #3601
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
Reset DAGMC history when reviving from source. #3601
Conversation
|
Happy to report that this fixed my issue, including on a rebased branch with transformation BC support (#3402). Tally results seem to agree within uncertainty compared to what I saw previously. The minor discrepancy may be due to new features/bug fixes with the way weights are now handled in conjunction with source sampling. Thanks for this quick fix @pshriwise! |
gonuke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it definitely makes sense and should fix the problem.
Thanks for the quick solution @pshriwise
|
I've been working with @connoramoreno on adding a minimal test using cube geometries designed to demonstrate this failure mode so we understand it well and don't have a regression. And I'd like to include it here. |
…esn't cause a problem in DAGMC models
…rrent set of weight windows still elicits the bug we're concerned with here.
|
To provide a quick update on this. The CI failure seen for 2031255 is caused by an issue with how DAGMC performs volume checks when configured to use the MOAB For clarity, this doesn't happen when using DAGMC w/ double-down enabled due to differences in the way it handles point in volume ray queries. This is why those utilizing this branch, including myself, likely haven't encountered this problem. An easy solution for us to avoid this issue was to rotate the cubes about 45 degrees about each axis to ensure that volume bounding box faces aren't coincident in which case this issue doesn't arise. This should help us isolate the test case to address only the bug at hand here and not be affected by this issue, which will be addressed (and tested) in that project instead. More info on the MOAB raytracer issue I'm referring to and the proposed fix here. |
| def execute_test(self): | ||
| # this test merely needs to run OpenMC without error | ||
| try: | ||
| self._run_openmc() | ||
| finally: | ||
| self._cleanup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this test is literally just running OpenMC to make sure it completes, I don't think it really belongs in regression_tests. Can you 1) move it to the unit_tests directory and 2) construct the model using the Python API since it is simple enough? Also, is there any way we can get the .h5m file to be smaller? 57 kB is not huge but more than I would normally like to add for a new test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. It does check for a regression of this particular pathology with DAGMC geometry, but you're right it doesn't really fit into the regression test suite. I've moved it into the unit tests under weight windows.
I'll think on how to reduce the size of the .h5m file but it's already about as minimal as it can get -- 3 cubes (8 vertices, 12 triangles each) with one material assignment. I could remove one of the cubes but I doubt it would change the file size substantially. Another option might be to adapt one of the other DAGMC models in the repo. However, the key to this model is that several surfaces are very close to each other, with the outer surface set with a vacuum boundary condition. The current set of DAGMC models in the repo don't meet that condition.
paulromano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pshriwise!
Description
A particle's DAGMC history should be reset when particles are being sourced from the secondary bank. A history from a previous particle can cause incorrect intersection determination in DAGMC as any previously intersected faces in the history at the time of sourcing from the secondary bank may cause surface intersections to be erroneously ignored.
A hypothetical related to #3600 would be the following:
This probably hasn't occurred very often because more splitting occurs in materials with higher collision density and collision events reset the particle history. That said, a MWE wasn't terribly difficult to construct and I'm glad to be able to address the problem.
Fixes #3600
Checklist
I have made corresponding changes to the documentation (if applicable)