Differentiate materials in DAGMC universes #3056

bam241 · 2024-06-22T15:23:22Z

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

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)

this should address #2923

This work has been sponsored by NAAREA.

pshriwise

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...

openmc/lib/dagmc.py

openmc/model/model.py

openmc/universe.py

include/openmc/cell.h

openmc/geometry.py

…benefit from them

bam241 · 2024-10-08T09:13:54Z

@pshriwise I think I have addressed all your comments.

pshriwise

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.

pshriwise · 2024-10-08T17:10:35Z

src/cell.cpp

@@ -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 are a couple usages of geom_type_ directly in particle.cpp (and perhaps elsewhere). Can you update those lines as well please?

changed all geom_type_ call to geom_type()
changed suface.h accordingly

openmc/lib/dagmc.py

openmc/model/model.py

openmc/cell.py

openmc/dagmc.py

pshriwise · 2024-10-08T17:38:38Z

openmc/dagmc.py

+                f"Material name '{mat_name}' not found in DAGMC file")
+        cv.check_iterable_type('material objects', overrides, str)
+
+        self.material_overrides[mat_name] = overrides


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.

This should allow to override per cell_id and material name

pshriwise · 2024-10-08T17:39:45Z

openmc/dagmc.py

+            None
+        """
+        for cell in self.cells.values():
+            if cell.n_instances > 1 and isinstance(cell.fill, Iterable):


Is there a reason this is restriced to cells with more than one instance?

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.

Co-authored-by: Patrick Shriwise <[email protected]>

bam241 · 2024-10-11T14:38:51Z

@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 · 2024-10-12T09:06:29Z

@pshriwise here is an other illustration of the CI inconsistance:
bam241#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

include/openmc/dagmc.h

pshriwise · 2024-10-28T16:45:59Z

openmc/lib/__init__.py

@@ -68,6 +68,7 @@ def _uwuw_enabled():
 from .math import *
 from .plot import *
 from .weight_windows import *
+from .dagmc import *


Let's either import the required function names directly or include an __all__ paramter in dagmc.py to prevent symbol/name pollution from the wildcard import here.

openmc/model/model.py

pshriwise · 2024-10-28T17:01:54Z

src/dagmc.cpp

@@ -211,15 +230,44 @@ void DAGUniverse::init_geometry()
    if (mat_str == "graveyard") {
      graveyard = vol_handle;
    }
-
    // material void checks
    if (mat_str == "void" || mat_str == "vacuum" || mat_str == "graveyard") {
      c->material_.push_back(MATERIAL_VOID);
    } else {
      if (uses_uwuw()) {
        uwuw_assign_material(vol_handle, c);


How do we want the overrides to work when UWUW materials are present? I think that we'd want them to override UWUW material assignments as well.

I did something about this:

first check if override is path by cell ID,

then check if uwuw is used to recover the material name (as override key),

finally use the mat_str as override key

in all 3 cases, use to legacy assignment to override mat assignment.

src/dagmc.cpp

pshriwise · 2024-10-28T17:13:50Z

tests/unit_tests/dagmc/test_model.py

+    mats["Water"].add_s_alpha_beta("c_H_in_H2O")
+    mats["Water"].name = "Water"
+
+    p = pkg_resources.resource_filename(__name__, "dagmc_differentiate_mat.h5m")


In the interest of minimizing the number of binary files in the repo and it's overall size, would one of the other existing DAGMC files in the regression tests work for this test instead?

pshriwise · 2024-10-28T17:48:45Z

src/dagmc.cpp

+  *n = dag_cell_ids.size();
+}
+
+extern "C" int openmc_dagmc_universe_get_num_cells(int32_t univ_id, size_t* n)


We'll want all of these functions to return proper error codes to satisfy the C interface.

pshriwise · 2024-10-28T17:51:57Z

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

Co-authored-by: Patrick Shriwise <[email protected]>

…ation

bam241 · 2024-10-31T15:19:10Z

@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 · 2024-10-31T15:21:28Z

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 force-pushed the dagmc_decoupling branch from b30d703 to 5ec8978 Compare July 24, 2024 04:38

bam241 and others added 14 commits July 31, 2024 18:35

start commenting

903d114

test

66a2be4

up

afd5a96

new state

538313a

new state

e2c41c6

removing print

ceb12fb

removing unnecessary comment

42dc9c3

fix

2102210

add differente_mats into model

52dcd3f

syntax

b6df271

docstring

8f827cc

tmp commit

a71e488

computer change

6a80008

computer change

394d585

bam241 force-pushed the dagmc_decoupling branch from 5ec8978 to 394d585 Compare July 31, 2024 16:38

Baptiste Mouginot and others added 10 commits August 2, 2024 11:01

in progress

4ec6b2b

in progress

5dc39fb

temp

0a1e6c5

temp

7aa6f64

up

f7a9b23

cleaning printout

f3acb0b

adding openmc_get_dagmc_cell_ids to compilation without DAGMC

9bfab40

Adding some test for dagmc_split

635f1f5

put test restriction on dagmc model test if there is no DAGMC

44ae1ad

adding missing import

25d29a6

bam241 marked this pull request as ready for review August 12, 2024 15:30

bam241 requested review from pshriwise and paulromano as code owners August 12, 2024 15:30

pshriwise reviewed Aug 15, 2024

View reviewed changes

factoring method form Universe to UniverseBase, do DAGMCUniverse can …

b6ad4b7

…benefit from them

pshriwise mentioned this pull request Oct 3, 2024

Add test for plotting with DAGMCUniverse.plot #3139

Open

bam241 added 3 commits October 5, 2024 16:30

allow assignement overload per cell_id for DAGMC universe

8b0906d

Merge remote-tracking branch 'upstream/develop' into dagmc_decoupling

c25d34e

reset fmt to upstream/develop val

569348e

pshriwise requested changes Oct 8, 2024

View reviewed changes

bam241 and others added 3 commits October 9, 2024 09:25

Apply suggestions from code review

a9e0306

Co-authored-by: Patrick Shriwise <[email protected]>

adressing third round of comment from @pshriwise

dae5527

Merge remote-tracking branch 'upstream/develop' into dagmc_decoupling

182d991

Adding missing import for input_path

50cfcab