Differentiate materials in DAGMC universes

[bam241]

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

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

this should address #2923

This work has been sponsored by NAAREA.

[bam241]

@pshriwise I think I addressed most of you comments (on the python side, didn't find the time to touch the 1 or 2 comments about the cpp..).

I didn't revolve the conversation so you can keep track of them.

One remaining question is: shall I "refactor" BaseUniverse/Universe/DAGMCUniverse to avoid redundancy as much as possible?

I am also happy to implement a DAGMC.py if you fell it is needed, while I am at it, it is probably less work than to go back at it later :)

let me know

[pshriwise]

One remaining question is: shall I "refactor" BaseUniverse/Universe/DAGMCUniverse to avoid redundancy as much as possible?

I think adding abstract methods for the cell modification methods would be good as a start. I do wonder if we might be able to rely on the same implementation in Universe and DAGMCUniverse for those methods. It might be worth a go to see if the implementation gets messy handling both the Cell and DAGMCCell types. We could make a base class, CellBase but I have a feeling that would add a level of inheritance that we don't really need b/c, as the current class name suggests, the DAGMC cells are pseudo cells.

I am also happy to implement a DAGMC.py if you fell it is needed, while I am at it, it is probably less work than to go back at it later :)

If you're up for it I think that would be a good improvement from a codebase organization standpoint! Thanks for being willing to do so ❤️

[bam241]

@pshriwise I think I have addressed all your comments. (factorisation of the different Universe methods between UniverseBase, Universe and DAGMCUniverse) and created a specific dagmc.py that contains DAGMCUniverse and DAGMCCell

---

You mentioned something about improving the xml nodes, shall we go back to it ?

---

Also there might be a problem I spotted when doing my tests. When using model.differentiate_depletable_mat("match cell"), the volume of the cell is applied to each materials, When using the DAGMCUniverse to fill multiple Cells the DAGMCCells have multiple instances, and the volume of them is: DAGMCCell.Volume = Real_Cell_volume * DAGMCCell._n_instance (where Real_Cell_volume is the actual volume of the DAGMCCell)

I am guessing this is not a specific DAGMC problem: if we use any Universe (DAGMC or not) to fill multiple Cells of the Geometry, one will end up with the same issue. Shall we do something about this ? Add a warning ?

---

Also, when using model.differentiate_mats and running new volume calculation I need to run model.init_lib() multiple time, to resync the cpp. Shall I add a method to sync the cpp with the geometry/model change in python ? and call it if the cpp_lib is initialised in the method that would change the model?

i.e.: to run a volume calculation on the DAGMCCells I need:

1. definition of the geometry in Python
2. model.init_lib() (to recover the information of the DAGMCCells)
3. definition of the Volume calculation on the DAGMCCell
4. model.init_lib() (to update the xml with the new volume calculation information)
5. model.calculate_volumes() ...

[bam241]

@pshriwise following up on the CI issue I mentioned Friday: as you can see in the commit history: 3197ee4 passed, but 3a7c19c didn't.

the only difference is: openmc/model/model.py

@@ -1050,10 +1050,6 
            volume equally between the new materials, 'match cell' sets the
            volume of the material to volume of the cell they fill.
        """
-       # if diff_volume_method not in ['divide equally', 'match cell']:
-       #     raise ValueError(
-       #         f"diff_volume_method must be 'divide equally' or 'match cell', "
-       #         f"not '{diff_volume_method}'")
        # Count the number of instances for each cell and material
        self.geometry.determine_paths(instances_only=True)

[pshriwise]

I wanted to note that the ability to differentiate materials in the Model.differentiate_mats without applying material volumes added in this PR might help with those looking to determine the volume of cell instances (albeit in a roundabout way by performing a material volume calculation afterward)

https://github.com/openmc-dev/openmc/issues/2367

[bam241]

@pshriwise, I think I have addressed all your comments.

the only remaining is the reference the materials per id in the override_material dict

[bam241]

@pshriwise I think I have addressed all your comments.

[bam241]

@pshriwise I think I addressed your last round of comments.

As I moved the DAGMCUniverse from openmc.universe to openmc.dagmc and shuffled some methods from Universe to UniverseBase , in the mean time openmc.universe has been updated on develop, It is difficult to make sure I didn't miss any update to DAGMCUniverse, Universe and UniverseBase while resolving the conflict.

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 :)

[bam241]

@pshriwise here is an other illustration of the CI inconsistance: https://github.com/bam241/openmc/pull/1

I have a PR with this branch on my fork, the [Python 3.10 (omp=y, mpi=y, dagmc=y, ncrystal=, libmesh=, event= vectfit=)](https://github.com/openmc-dev/openmc/pull/3056#logs) test failed after 6h. while here it worked in 1h

[pshriwise]

Another set of comments for you @bam241. Sorry if they're a little hard to track -- should've done a proper review.

This is looking quite good! Just a few spots to polish up and I think it'll be ready

[bam241]

@pshriwise I added an other test, to check more extensively what is done, I can probably factorise some code between the 2 tests, will do today or tomorrow.

Not that I had to remove the test that checked that the number of instance of the cells matched the number of instance of the override when applying the material assignment override in dagmc.cpp. The number of instances of the Cell is determine after loading the DAGMC model, therefore it is 0 when loading the DAGMC geometry... so we can't test against it at this moment...

[bam241]

I also, make an addition split method, to allow splitting based on an arbitrary delim, some material names (i.e. in the actual dagmc.h5 used in the test) have space in it, therefore space is not a proper delimiter to separate the list of material names use for the override. I used ; instead

[bam241]

@pshriwise this is ready for a other round of review :)

[bam241]

@pshriwise I think I addressed to all your questions or comments.

I left 1 unresolved comment, so you can "ok" how I have addressed your question about the uwuw assignment.

[bam241]

@pshriwise I have addressed your last comments,

let me know if you have more :)

[pshriwise]

There's a lot of nice work here. My remaining concern is the structure of the material_overrides attribute. I think this data structure should only accept Cell IDs as keys. The values can be a single Material instance or in the case that cell instances should be filled with different materials (or material clones), the value is an iterable of Material instances with length equal to the number of Cell instances. This provides a direct and natural mapping for how cell materials are assigned and is analogous to how this works for CSG cells in the XML files.

To me, using materials as keys forces this awkward check regarding how cells look up what material alias should be used (currently relying on special naming of materials). If there's an iterable of Material's as a value the length of that entry limits is application to a specific set of cells and I can see us getting into trouble there.

What do you think @bam241? Am I missing something?

Either way, documentation should be updated to be consistent once we come to a conclusion on this.

[bam241]

There's a lot of nice work here. My remaining concern is the structure of the material_overrides attribute. I think this data structure should only accept Cell IDs as keys. The values can be a single Material instance or in the case that cell instances should be filled with different materials (or material clones), the value is an iterable of Material instances with length equal to the number of Cell instances. This provides a direct and natural mapping for how cell materials are assigned and is analogous to how this works for CSG cells in the XML files.

To me, using materials as keys forces this awkward check regarding how cells look up what material alias should be used (currently relying on special naming of materials). If there's an iterable of Material's as a value the length of that entry limits is application to a specific set of cells and I can see us getting into trouble there.

What do you think @bam241? Am I missing something?

Either way, documentation should be updated to be consistent once we come to a conclusion on this.

The only issue I see with that is the obligation to use the openmc.lib.

if we allow using the dogma material name assignment as overriding key, it would allow user to override materials assignment manually without getting the DAGMC Cell ID.

If we make the openmc.lib mandatory for this feature then we could allow user to pass it by name to the interface, and search to which DAGMC Cell it corresponds, so the user don't have to do the search by himself

[bam241]

I let you decide if making openmc.lib mandatory is ok or not.

[pshriwise]

I let you decide if making openmc.lib mandatory is ok or not.

I think it's reliance on openmc.lib is okay. Our plotter capabilities rely heavily on this currently and so do the depletion capabilities of the Model class, so there's a precedent for that. In the long term this probably isn't ideal and we should move to a Python API that connects more seamlessly to the C++ library, but I don't think we should draw the line at this capability by any means.

[bam241]

I let you decide if making openmc.lib mandatory is ok or not.

I think it's reliance on openmc.lib is okay. Our plotter capabilities rely heavily on this currently and so do the depletion capabilities of the Model class, so there's a precedent for that. In the long term this probably isn't ideal and we should move to a Python API that connects more seamlessly to the C++ library, but I don't think we should draw the line at this capability by any means.

@pshriwise I think I have addressed all your comments.

I refactored the python side a bit, to force all the addition to the override dictionary to go through the add function, where all the checks are.

You now can make an addition to the override dictionary via:

• material_name => check if name is in the DAGMC Geometry => get all corresponding Cell and add them as key
• cell_id => Check if the CellID is in the DAGMC Geometry => Add the corresponding Cell as a single Key
• Cell => Check if the Cell is in the DAGMC Geometry => Add the Cell as a single key

To deal with the capability to override a Material by a single Material, if a single material is provided to the add_override method, it is embedded in a List to keep the Iterrable format which simplifies the checking the type of the item in the list and building the xml file.

[bam241]

Also I had to keep a prefix for the xml attribute as it seems that an attribute cannot be a int. so I kept a "Cell_" prefix which I remove on parsing the xml in the cpp.

[pshriwise]

Also I had to keep a prefix for the xml attribute as it seems that an attribute cannot be a int. so I kept a "Cell_" prefix which I remove on parsing the xml in the cpp.

Can you elaborate on this for my sake? They cannot be int's, true, but why not convert the integer to a string without the prefix? For a space delimited list of Material IDs we should be able to parse those easily in C++ as we do for filter bins no?

[pshriwise]

I let you decide if making openmc.lib mandatory is ok or not.

I think it's reliance on openmc.lib is okay. Our plotter capabilities rely heavily on this currently and so do the depletion capabilities of the Model class, so there's a precedent for that. In the long term this probably isn't ideal and we should move to a Python API that connects more seamlessly to the C++ library, but I don't think we should draw the line at this capability by any means.

@pshriwise I think I have addressed all your comments.

I refactored the python side a bit, to force all the addition to the override dictionary to go through the add function, where all the checks are.

You now can make an addition to the override dictionary via:

• material_name => check if name is in the DAGMC Geometry => get all corresponding Cell and add them as key
• cell_id => Check if the CellID is in the DAGMC Geometry => Add the corresponding Cell as a single Key
• Cell => Check if the Cell is in the DAGMC Geometry => Add the Cell as a single key

To deal with the capability to override a Material by a single Material, if a single material is provided to the add_override method, it is embedded in a List to keep the Iterrable format which simplifies the checking the type of the item in the list and building the xml file.

Nice! Yeah I like this. There's something to be said for wanting to replace material A in the DAGMC file with material B. Just to clarify, the entries in the data structure are still Cell's/Cell IDs as keys and a list of Material's / Material IDs as values?

[bam241]

In the data structure yes: if material A is assigned initially in Cell ids [X,Y,Z] the data structure will look like: {"Cell_X":[B], "Cell_Y":[B], "Cell_Z":[B]},

The "Cell_" prefix is automatically taken out when parsing the XML file on the cpp side. As the Cell ids are attribute in the XML file it seems the they can't be int only( I tried to add them as str(id) but it apparently wanted some chars...)

The user could use the following method to apply the replacement: mat_B = openmc.Material() DAGMCUniverse->add_material_override(key="A",override=mat_B) or DAGMCUniverse->add_material_override(key=Cell_X,override=mat_B) DAGMCUniverse->add_material_override(key=Cell_Y,override=mat_B) DAGMCUniverse->add_material_override(key=Cell_Z,override=mat_B) or DAGMCUniverse->add_material_override(key=Cell_X.id,override=mat_B) DAGMCUniverse->add_material_override(key=Cell_Y.id,override=mat_B) DAGMCUniverse->add_material_override(key=Cell_Z.id,override=mat_B)

Value of the override can be Material or material Ids in the override_materials, and Material names, or materials Ids in the xml file, (default in the XML file would be the Material Ids).

[pshriwise]

In the data structure yes: if material A is assigned initially in Cell ids [X,Y,Z] the data structure will look like: {"Cell_X":[B], "Cell_Y":[B], "Cell_Z":[B]},

The "Cell" prefix is automatically taken out when parsing the XML file on the cpp side. As the Cell ids are attribute in the XML file it seems the they can't be int only( I tried to add them as str(id) but it apparently wanted some chars...)_

I'm a little confused by this, because we currently write cell IDs as XML node attributes

https://github.com/openmc-dev/openmc/blob/fbb115921c71997d37e62748a077bdd369667786/openmc/cell.py#L648-L649

Maybe we aren't thinking about the same thing here. To be specific what I'm thinking is that the material_overrides node would look something like:

<dagmc_universe filename="dagmc.h5m" id="1">
  <material_overrides>
    <cell id="1">
      10 11 12
    </cell>
    ...
  </material_overrides>
</dagmc_universe>

And so in the Python data structure it would look like:

>>> print(dagmc_universe.material_overrides)

{1: [10, 11, 12], ...}

The user could use the following method to apply the replacement: mat_B = openmc.Material() DAGMCUniverse->add_material_override(key="A",override=mat_B) or DAGMCUniverse->add_material_override(key=Cell_X,override=mat_B) DAGMCUniverse->add_material_override(key=Cell_Y,override=mat_B) DAGMCUniverse->add_material_override(key=Cell_Z,override=mat_B) or DAGMCUniverse->add_material_override(key=Cell_X.id,override=mat_B) DAGMCUniverse->add_material_override(key=Cell_Y.id,override=mat_B) DAGMCUniverse->add_material_override(key=Cell_Z.id,override=mat_B)

Value of the override can be Material or material Ids in the override_materials, and Material names, or materials Ids in the xml file, (default in the XML file would be the Material Ids).

Nice. Yeah I like this option for a material --> material override 👍🏻

[bam241]

@pshriwise I reshaped the xml layout. it now looks like:

    <material_overrides>
      <cell id="8" material="12;13;14;15"/>
      <cell id="9" material="16;17;18;19"/>
      <cell id="10" material="20;21;22;23"/>
    </material_overrides>

I kept the ";" separator between material values, as material name could be passed, so I thought semicolon was better than space. Let me know.

[pshriwise]

I kept the ";" separator between material values, as material name could be passed, so I thought semicolon was better than space.

I didn't catch that we might write material names to XML in our conversation above, sorry. Any reason not to limit the values of the overrides data structure to Material objects or IDs? If we have the name of a material, presumably the Material object is right there and we can pass it instead?

From

dagmc_univ.add_material_override(cell_a.id, mat.name)

To

dagmc_univ.add_material_override(cell_a.id, mat)

It's nice to be able to reference materials by name when marking up the DAGMC file in a CAD program, but the OpenMC Material object (partially by nature of having a name attribute) is equally rich.

[bam241]

didn't catch that we might write material names to XML in our conversation above, sorry. Any reason not to limit the values of the overrides data structure to Material objects or IDs? If we have the name of a material, presumably the Material object is right there and we can pass it instead?

Not particularly, as eventually the override materials is assigned using the legacy_assign_material which allows both assignment by name or by id. I thought it could be nice to support both.

I switched from ; separator the format to space separated. and updated the documentation accordingly.

so now xml looks like

    <material_overrides>
      <cell id="8" material="12 13 14 15"/>
      <cell id="9" material="16 17 18 19"/>
      <cell id="10" material="20 21 22 23"/>
    </material_overrides>

[bam241]

@pshriwise ready of an other round :)