Differentiate materials in DAGMC universes #3056
Conversation
There was a problem hiding this comment.
Not an easy task @bam241 and I think I see where you're going with it. My main concern with the current implementation is the amount of DAGMC-specific code in higher-level sections of the Python API. I think there's some refactoring we can do to avoid special logic in the Model and (maybe) Geometry classes.
I'd say this is optional for this PR, but now that we have additional DAGMC* classes appearing I wonder if it would be best to move them all into a dagmc.py file in the Python API. I thought doing this in the C++ code was nice as DAGMC constructs have their own special logic to some degree. It would reduce the size of our cell.py and universe.py files as well. I can tackle that afte we finish this PR though. Making an issue to track...
| instance_material_overrides.find(mat_str) != | ||
| instance_material_overrides.end()) { | ||
|
|
||
| for (auto mat_str_instance : |
There was a problem hiding this comment.
A check here that the number of entries in the override dictionary is the same as the cell size would probably be wrothwhile.
What happens if the material override dictionary need to apply to more than one cell with many instances? It appears that the material assignment would start over again at the beginning of the iterable.
There was a problem hiding this comment.
I think this question still stands? If I'm missing something can you point to the response or code change that resolves it?
There was a problem hiding this comment.
I might have miss understood you, if so sorry for marking this as resolved.
I added the test before the for loop.
L236-242
If the material overrides apply to multiple cells, that means that those cells shared the same initial material assignement in the same DAGMCgeometry.
I think I see where the problem is, this works to differenciate materials in case of multiple insertion of the same DAGMC Geometry, this does not aim to differenciate assignement of a single material to multiple cells of the same DAGMC Universe.
There was a problem hiding this comment.
Yeah exactly. If the multiple DAGMC cells uses the same material and those cells have multiple instances, it isn't clear to which cell the material overrides will apply. It seems as though it would be the same for both cells, resulting in the same material usage for those cells in each use of the DAGMC universe. In the end I don't think this will accomplish material differentiation as desired.
There was a problem hiding this comment.
That was the intended behaviour,
Not to change the number of materials used in a DAGMC geometry but to replace the assigned materials each time the DAGMC is inserted again.
So if two cells share the same material assignment in the h5 file, the overriding material will be apply to both cells.
To go back to the previous loop,
mat_A is assigned to Cell1 and Cell2 that are part of a DAGMC Geometry
so we have:
Cell1.material_[0] = mat_A
Cell2.material_[0] = mat_A
If we insert the DAGMC geometry 3 times and we have a override element such as: mat_A:[mat_A_0, mat_A_1, mat_A_2]
So
Cell1.material_[0] = mat_A_0
Cell1.material_[1] = mat_A_1
Cell1.material_[2] = mat_A_2
Cell2.material_[0] = mat_A_0
Cell2.material_[1] = mat_A_1
Cell2.material_[2] = mat_A_2
The goal was not to change the assignment defined in the DAGMC Geometry, but to differentiate the materials between different occurrence of the same DAGMC Geometry in the simulation
I am happy to discuss if we want to have a different behaviour, but I can see why differentiate material in a way that differentiation of a material assigned to multiple volumes of the DAGMC geometry, could be useful, but it could also be dangerous.
There was a problem hiding this comment.
Personally, I like option a. I know it leaves more room for error if the user makes manual modifications after the call to
diff_burnable_mats, but historically the design of OpenMC has favored flexibility in these cases.
When talking about user manual override of the material assignment I was not referring to user manual modification after the call to diff_burnable_mats but simply a user wanting to override some materials of the DAGMC geometry writing manually the DAGMCUniverse.material_overrides or using DAGMCUniverse.add_material_override
I think it is a nice feature to be able to "replace" material from the original DAGMC geometry by mapping the original name with the news materials (for either unique or multiple DAGMC occurrence). It is also nice that it is 100% Python so it does not require to have the openmc.lib enabled.
I also like the "vision" you have, when using diff_burnable_mats to create a mapping between DAGMCCell.id and the materials for each instances of this cell.
IMO, we need to enable both, and find a way to pass that information correctly to the cpp.
There was a problem hiding this comment.
Going back to data structures, what do you think of representing the overrides in a
DAGMCCell.fillattribute?
Do you mean when using the diff_burnable_mats ?
because we already write the override in the cell.fill.
For DAGMCCell, it is then used to generate the material_override node in the xml.
As to have find_buranle_mats only works with DAGMCCell when openmc.lib is enable, we could pass back the information directly to the cpp DAGMCCell.
Or add a method to pass it.
Is that what you meant ?
There was a problem hiding this comment.
When talking about user manual override of the material assignment I was not referring to user manual modification after the call to
diff_burnable_matsbut simply a user wanting to override some materials of the DAGMC geometry writing manually theDAGMCUniverse.material_overridesor usingDAGMCUniverse.add_material_override
I think it is a nice feature to be able to "replace" material from the original DAGMC geometry by mapping the original name with the news materials (for either unique or multiple DAGMC occurrence).
IMO, we need to enable both, and find a way to pass that information correctly to the cpp.
I think we're in agreement that we can and should support both use cases of material overrides we've outlined above. My main point was that I think we can use the same override data structure (be it with a dict on the DAGMCUniverse or using the DAGMCCell.fill attribute).
It is also nice that it is 100% Python so it does not require to have the
openmc.libenabled.
I don't quite see how we can do this without using the openmc.lib module at some point myself, but you have a way to accomplish this in mind, great!
There was a problem hiding this comment.
Do you mean when using the
diff_burnable_mats? because we already write the override in thecell.fill. ForDAGMCCell, it is then used to generate thematerial_overridenode in the xml.
Ah, yeah you're right. I was thinking that if the information is stored on the DAGMCCell objects then we're kind of duplicating it in the material_overrides attribute. So I was wondering if we might just populate the XML element from the DAGMCCell objects during export and not have an attribute on the DAGMCUniverse. However, I'm not sure how one would handle importing an XML file without generating DAGMCCell objects. I think we can look into that further down the line. Not super important at the moment.
As to have
find_buranle_matsonly works withDAGMCCellwhenopenmc.libis enable, we could pass back the information directly to the cpp DAGMCCell. Or add a method to pass it.Is that what you meant ?
This would be kind of nifty, but I don't think we need to worry about it at the moment.
There was a problem hiding this comment.
I added a new implementation for this.
basically in the python DAGMCUniverse before generating the xml node for the DAGMCUniverse, one update the override_materials dict with the cell.fill (if the cells have multiple instance and an Iterable fill, i.e. the DAGMCCell is inserted multiple times in the Geometry, and has a different fill for each instance.)
If an override_material has already been set overloading per material name, I decided to keep both.
And on loading the DAGMC geometry (C++) to prioritise the override per cell Id then override per material name then fall back to material assignment as stated in the DAGMC file.
Co-authored-by: Patrick Shriwise <[email protected]>
|
I wanted to note that the ability to differentiate materials in the |
… the cell number instance
bam241
force-pushed
the
dagmc_decoupling
branch
from
b4b4509
to
f6a7e76
Compare
|
@pshriwise, the only remaining is the reference the materials per id in the override_material dict |
| instance_material_overrides.find(mat_str) != | ||
| instance_material_overrides.end()) { | ||
|
|
||
| for (auto mat_str_instance : |
There was a problem hiding this comment.
I think this question still stands? If I'm missing something can you point to the response or code change that resolves it?
Co-authored-by: Patrick Shriwise <[email protected]>
bam241
force-pushed
the
dagmc_decoupling
branch
from
4e92a0c
to
ce99675
Compare
…benefit from them
|
@pshriwise I think I have addressed all your comments. |
There was a problem hiding this comment.
This is looking good @bam241! We're getting into the nit-picky phase now.
There are one or two sections that are DAGMC-specific in model.py and I wonder if we can figure out a way to build those into the DAGMC classes. I'll think on it some more, but then it'll fit in pretty seamlessly with native geometry in OpenMC.
| @@ -252,12 +252,12 @@ void Cell::to_hdf5(hid_t cell_group) const | |||
| // default constructor | |||
| CSGCell::CSGCell() | |||
| { | |||
| geom_type_ = GeometryType::CSG; | |||
| geom_type() = GeometryType::CSG; | |||
There was a problem hiding this comment.
There are a couple usages of geom_type_ directly in particle.cpp (and perhaps elsewhere). Can you update those lines as well please?
There was a problem hiding this comment.
changed all geom_type_ call to geom_type()
changed suface.h accordingly
| f"Material name '{mat_name}' not found in DAGMC file") | ||
| cv.check_iterable_type('material objects', overrides, str) | ||
|
|
||
| self.material_overrides[mat_name] = overrides |
There was a problem hiding this comment.
Does this support a single override as well as a list? Might be convenient to support that if one wants to override a material for all of the cell instances.
There was a problem hiding this comment.
This should allow to override per cell_id and material name
openmc/dagmc.py
Outdated
| None | ||
| """ | ||
| for cell in self.cells.values(): | ||
| if cell.n_instances > 1 and isinstance(cell.fill, Iterable): |
There was a problem hiding this comment.
Is there a reason this is restriced to cells with more than one instance?
There was a problem hiding this comment.
your comment made me thinking :)
You are right, if cell.fill is an Iterable then cell.n_instances has to have more than one instance.
I though about the eventuallity to remove restrictions on Iterable,(and cell.n_instance), but as all DAGMCcells, when sync get their fill property that would me filling the override materials dict with all the fill of all DAGMCcells overridden or not.
|
@pshriwise I think I addressed your last round of comments. As I moved the DAGMCUniverse from I would suggest extra caution when reviewing those part, in case I missed any (I have been careful... but I might have missed something) and would ask for as much swiftness as we can have to conclude this (without impacting the quality) to avoid any additional complicated conflict :) |
|
@pshriwise here is an other illustration of the CI inconsistance: I have a PR with this branch on my fork, the |
Description
This allows to differentiate materials in DAGMC universe when using the same DAGMC geometry to fill different CG cells
This provides a mechanism to overload material assignment embedded in the DAGMC geometry by providing a dict
{ref_mat_name:[mat_instances]}This allows enrich the
Model.differentiate_depletable_material()method from the Python API, to allows automatic differentiation of the depletable materials of a DAGMC geometry inserted multiple times.Checklist
this should address #2923
This work has been sponsored by NAAREA.