-
Notifications
You must be signed in to change notification settings - Fork 457
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
DAGMC CAD-based geometry as universes #1825
Conversation
Ooh very exciting :)
…________________________________
From: Patrick Shriwise ***@***.***>
Sent: Tuesday, April 27, 2021 10:12:21 PM
To: openmc-dev/openmc ***@***.***>
Cc: Davis, Andrew ***@***.***>; Review requested ***@***.***>
Subject: Re: [openmc-dev/openmc] DAGMC CAD-based geometry as universes (#1825)
@pshriwise<https://github.com/pshriwise> requested your review on: #1825<#1825> DAGMC CAD-based geometry as universes.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub<#1825 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AASTUSV3N6KP3ENVKEFXRCLTK4SDLANCNFSM43VYSAZA>.
|
In case anyone else is wondering, this seems to add pretty much zero overhead to non-DAGMC models. Seems like a really useful upgrade. |
73b23d7
to
e6f4d7f
Compare
328fb89
to
5b20fbb
Compare
Sorry for the initial CI failures here. Needed to make a correction to the properties of the |
@pshriwise is it worth me adding a rotation to this example #1800 . I assume that I just remove the reflecting surface (perhaps using the new dagmc_remove_tags package) and rotate 90, 180 and 270 degrees to make the complete geometry |
I'd welcome it! It would make #1800 dependent on this PR though. I'll leave it up to you as to whether or not you're ok with waiting on these changes to get that example in. |
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.
Looks mostly good here, I think a few extra new lines, some extra comments and its good :)
// determine the next universe id | ||
int32_t next_univ_id = 0; | ||
for (const auto& u : model::universes) { | ||
if (u->id_ > next_univ_id) next_univ_id = u->id_; |
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.
will u->id_
always be sorted smallest to largest?
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.
It isn't a requirement for model::universes
, no. Are you concerned the next ID isn't correct?
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.
just wondering if it leads to any issues, like does it matter if the order isnt monotonically increasing?
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.
The IDs can be in any order here. Whenever we find one that's bigger, we update next_univ_id
. If we happened to find the largest ID first, we'd only update that variable once.
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.
Are you happy with this implementation @makeclean?
src/dagmc.cpp
Outdated
@@ -279,39 +232,32 @@ void load_dagmc_geometry() | |||
} | |||
|
|||
if (!graveyard) { | |||
warning("No graveyard volume found in the DagMC model." | |||
warning("No graveyard volume found in the DagMC model. " |
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.
Do you need a graveyard even in a universe based model? Do OpenMC cells not bound this?
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.
The only case where you now need one is if the DAGMC model is the root universe of the Geometry. I'll add some detection for that so this warning doesn't appear when it shouldn't. Thanks!
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.
Reorganized the code a bit so we can perform this check in 648054f. Hopefully this is what you had in mind!
Looks like code block 17 from the cad-based-geometry.ipynb has the old |
I've been using the branch on an ARC reactor model and have reproduced the example pictures with a different geometry (still 90 degree rotated 3 times). I've found the ability to turn off the auto assign id numbers useful Actually this feature really saved me and was exactly what I needed to get some simulation results. Perhaps this is unrelated to this PR but one thing I noticed was that the terminal prints out a lot of (perhaps too much) information when plotting an DAGMC geometry using this method
|
Yeah, I'm seeing that too. Thanks for sharing this. Appears to be a warning about a corner case in MOAB's triangle intersection routine. I'll have to look into that separately. |
Thanks! Just took care of that one. |
…es with other CSG objects so far!
… and cell indices.
…s not contain a graveyard volume.
302f12b
to
648054f
Compare
I believe I've addressed all of your comments @paulromano, @makeclean. Let me know if I've missed anything! I did some further testing using the CSG and DAGMC ATR models originally used for comparison in our DAGMC implementation.
I ran the CSG and DAGMC models w/ 200000 particles per batch, 500 batches, 20 inactive. I then replaced the external boundary volume of the DAGMC model (a.k.a. the graveyard volume in DAGMC terms) with CSG planes and a vacuum BC. I'd expect that the flux comparison between the DAGMC model and the DAGMC model bounded by CSG planes would be exactly the same, which is what we see here. I don't expect the comparison between the CSG and DAGMC models to be the same due to small differences in the geometry caused by the CAD tessellation, however. The results looked reasonable to me, with the largest relative differences in the flux occurring near the edge of the core where the variance in either flux tally is highest. Overall, very few of the flux voxels were statistically different. I then inserted CSG X and Y planes at the origin w/ reflective BC's to test individual quadrants of the model.
Again, the results looked reasonable when comparing the the CSG model for each quadrant to the DAGMC model. I've included the results of this comparison for quadrant 4: There were a few statistically different results in the mesh, but, as I mentioned above, this isn't unexpected due to the fact that the geometries are in fact different. |
@pshriwise Thanks for incorporating all the feedback. I have a few more requests:
|
…verses are read into geometries from XML files.
…ced and exported to XML from a summary file.
Thanks for @paulromano! Docs have all been updated. I've updated the export/import code for XML so that it handles DAGMC universes properly and can reproduce the same I noticed along the way that surface names weren't being picked up in |
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 for the latest round of updates. A few more requests based on those changes... we're getting close!
Co-authored-by: Paul Romano <paul.k.romano@gmail.com>
…are limited to 52 characters.
…ith standard Universes.
…r one of these by accident.
Co-authored-by: Paul Romano <paul.k.romano@gmail.com>
@makeclean Do you have any further comments on this PR? |
LGTM thanks @pshriwise |
We currently only support DAGMC models where the entire geometry is represented as CAD-based tesselations. This PR adds a DAGMC Universe derived from the standard CSG universe. This allows one to construct geometries from both CAD and CSG. This enables repeated CAD geometries in OpenMC like this lattice of DAGMC pincells from our regression tests
Or, as a more interesting case, one can rotate a section of a tokamak into a set of CSG cells to model the entire device with a smaller DAGMC memory footprint. (Thanks @shimwell for providing this quarter-tokamak model as a demonstration.)
Here is the entire tokamak model in Cubit:
We can add this model as a fill to the appropriate cells and rotate the model 90 degrees to get this X-Y view:
Adding two more cells and rotations gives use the entire device:
Input Changes
These updates do require some changes to the current format when the DAGMC model represents the entire geometry. A
geometry.xml
file is now always required. To represent a geometry composed of a single DAGMC model as we do now, ageometry.xml
file that gives the same behavior should containwhere the
id
is the universe ID and the filename is the name of the DAGMC model file.Note that the
filename
parameter remoes the restriction that DAGMC files have the namedagmc.h5m
.Notable code changes
DAGUniverse
classCell::geom_type
andSurface::geom_type
settings::dagmc
parameter from the CAPI and Python APIUniverse::find_cell
method#ifdef DAGMC
's throughout the code thanks to the addition of theGeometryType
enumID management
One difficulty of mixing CSG and CAD geometry is the unknown ID spaces of the CAD geometry before simulation initialization. The
DAGUniverse
class on the Python side has an option,auto_geom_ids
, to automatically set cell and surface IDs outside of the CSG cell & surface space to avoid conflicts. If this option is not enabled, then an error will occur if the ID spaces overlap.A similar option,
auto_mat_ids
, is also available for materials to address the case where some of the DAGMC universes are using UWUW materials (material definitions embedded in the.h5m
along with the geometry.Particle tracking considerations
Coincident DAGMC/CSG surfaces
Thanks to our preference for hitting surface at higher levels in the geometry, this is hasn't been a problem in the models I've used for testing.
Managing a particle's ray history
The ray history is a record of facets hit by DAGMC rays to avoid duplicate hits when coincident with a surface or reflecting from a surface. This history is reset when
Now that a particle can move directly from one DAGMC universe to another, another case is added for
DAGUniverse
For more details on this implementation, please see this google doc w/ the proposal for this work.