Refactoring Material to not subclass Composite #1062

john-science · 2023-01-04T18:00:43Z

Description

This PR refactors the Material class to no longer be a subclass of Composite.

It also removes (most, not all) of the statefulness in ARMI Materials. For instance, I removed all Parameters from Material. Some of those I copied over to a simpler class attribute (mat.p.massFrac became mat.massFrac). And some I was able to delete entirely (like yieldStrength and atomFracDenom).

Also, I should note that three parameters (uFrac, puFrac, and zrFrac) applied only to FUEL materials, but were defined by default for all materials. That was fixed.

Stateful things I haven't removed in this PR (because they are going to be tricky):

ARMI supports Custom materials, which are obviously stateful by design.
ARMI supports custom isotopics for fuels, which again is a stateful idea.
The Material class still has a self.parent, which is necessary for a whole LOAD of logic to do with Material.getNuclides(). (@ntouran Please note this proved to be a big enough change that if we want to remove it, it will be it's own PR.)

@drewj-usnctech @sombrereau In any repo I can see, I am making PRs to match this one. But if you have a private repo I can't see, 99% of the changes are just to switch calls to mat.p.thing to mat.thing. (Once, a material outside ARMI needed an __init__() method, that it didn't have before.)

Checklist

This PR has only one purpose or idea.
Tests have been added/updated to verify that the new/changed code works.

The code style follows good practices.
The commit message(s) follow good practices.

The release notes (location doc/release/0.X.rst) are up-to-date with any bug fixes or new features.
The documentation is still up-to-date in the doc folder.
The dependencies are still up-to-date in setup.py.

john-science · 2023-01-04T18:03:01Z

@drewj-usnctech Just FYI

drewj-usnctech · 2023-01-04T22:34:39Z

Thanks @john-science

Quick skim and nothing seems to jump out to me here

armi/materials/material.py

ntouran · 2023-01-05T19:15:59Z

armi/materials/material.py

@@ -73,7 +87,8 @@ class Material(composites.Composite):
    with information about thermal scattering."""

    def __init__(self):
-        composites.Composite.__init__(self, self.__class__.name)
+        self.parent = None


Why do materials still have parents? They shouldn't, since they're no longer in the hierarchical tree.

Remove or justify with implementation docs.

It appears to me that the mass fractions in all ARMI materials are designed to be changed by the user's choice of nuclides. And right now those nuclide choices come from the parent.

Can you enlighten me on this? Because these nuclides are also the reason materials have parameters.

ARMI materials seem to change a LOT based on the nuclides present in ARMI, and those nuclide are not static in ARMI.

Materials used to be the place where composition (defined as number density vectors of elements and nuclides) was stored. We moved that to the Component's numberDensities param long ago.

However, we still initialize this param based on the Materials that users add to their problem via the Material.setDefaultMassFracs method, which sets Material.p.massFrac. This is indeed (and unfortunately) a bit coupled to nuclide choices, since sometimes users want to model Carbon while other times they want to model Carbon-12.

So may be we need an intermediate ticket here, with the goal of: "Decouple composition definitions from neutronics library assumptions".

Material definitions should be static and evergreen, not dependent in any way on user modeling settings. This should include initial material composition, and probably also the process of building Component numberDensities, and potentially even the evolution of composition vs. time from transmutation and decay and reprocessing and everything else. The user modeling settings that map compositions to specific subsets of nuclides based on neutronics library needs should be isolated to the neutronics physics modules. For example, if a neutronics input is being built for a code that only has C-12 but the composition has Carbon, a mapping between carbon and C-12 should happen as the input is being written.

At this point, changing setDefaultMassFracs to getDefaultMassFracs in a way that just returns the data might be workable, and just eliminate Material.p.massFrac altogether. We'll have to see how much code accesses p.massFrac and assess how exactly to pull off an upgrade like this.

Okay, the list of parameters that Material needs is short. I just removed two with no trouble, and a third is only used in UThZr.

I think this PR could probably be merged in as-is.

But for now, let me see if I can attack the situation with a process like this:

The original PR, just removing the Material(Composite) subclass.

Remove all the Material Parameter except the massFrac ones.

See if it's possible to remove Material.p.massFrac

Finally remove the Material metaclass.

I think (3) above is by far the hardest. So I will see how far I get and if that needs to be its own PR. I'm experimenting now.

Okay, I have made progress. I have removed all Parameters from Material and FuelMaterial except massFrac.

I'm hoping I'll remove that today, and we can re-open this PR.

(I have also prepped 7 parallel PRs to update downstream repos to match this one.)

Done.

Okay, I removed a lot of code, so I'm sure the code coverage will go down. BUT Material not longer has any Parameters.

ok, please just add docs in the code explaining why there's a parent and what's to be done about it.

ntouran · 2023-01-05T19:18:03Z

armi/materials/material.py

+    def backUp(self):
+        """Empty because statelss materials shouldn't use cache"""
+        pass
+
+    def restoreBackup(self, paramsToApply):
+        """Empty because statelss materials shouldn't use cache"""
+        pass
+
+    def clearCache(self):
+        """Empty because statelss materials shouldn't use cache"""
+        pass
+
+    def _getCached(self, name):
+        """Empty because statelss materials shouldn't use cache"""
+        return None
+
+    def _setCache(self, name, val):
+        """Empty because statelss materials shouldn't use cache"""
+        pass


Can we just delete all these?

Spelling error: stateless

Big picture: have you measured the performance impact of not caching the material properties vs. temperature? That was a hugely major performance bottleneck we identified through formal profiling of test cases.

We need to see a profiler trace of loading a reactor and printing out all number densities in order to judge this. Can you paste the before/after highlights of the profiler results as a comment to this PR?

Two questions:

Isn't a cache a "state" for the class?

In downstream repos, we have temperature-dependent densities. These caches will preserve the first value they get forever. Shouldn't that lead to incorrect material densities?

I will look into this more once I have fully removed the Parameters from Materials.

Done.

Since Material no longer subclasses Composite, I had to mostly copy them over from the Composite class. But now they aren't blank here any more.

armi/materials/mgO.py

armi/materials/molybdenum.py

armi/materials/thoriumOxide.py

drewj-usnctech · 2023-01-12T16:17:58Z

In any repo I can see, I am making PRs to match this one. But if you have a private repo I can't see, 99% of the changes are just to switch calls to mat.p.thing to mat.thing. (The only other change I've seen is a couple materials outside ARMI needed an init() method, that they didn't before.)

Thanks for the heads up @john-science 👍

ntouran

looks great, thanks! Just some minor comments.

ntouran · 2023-01-12T17:45:16Z

armi/materials/custom.py

@@ -40,7 +40,7 @@ def __init__(self):
        by a constant user-input density.
        """
        Material.__init__(self)
-        self.p.density = 1.0
+        self.customDensity = 1.0


kind of a minor point but if we're renaming things like this it might be wise to make them supernames like customDensityInGPerCC. As it turns out, putting units in names is one honkin good idea, especially for super generic terms like 'density'

ntouran · 2023-01-12T17:46:36Z

armi/materials/material.py

@@ -73,7 +87,8 @@ class Material(composites.Composite):
    with information about thermal scattering."""

    def __init__(self):
-        composites.Composite.__init__(self, self.__class__.name)
+        self.parent = None


ok, please just add docs in the code explaining why there's a parent and what's to be done about it.

armi/materials/material.py

…30/assembly-1d-parent-class * terrapower/main: Updating B4C docstring for assumed TD frac (terrapower#1117) bug fix for xsGroup manager w/ coupling (terrapower#1118) Add neutronics category to linPow parameter (terrapower#1119) Refactoring Material to not subclass Composite (terrapower#1062) Exposing tight coupling to `OperatorSnapshots` (terrapower#1113)

john-science added 11 commits January 3, 2023 10:37

Refactoring Material to not subclass Composite

dd0d677

Making Material Meta class - for parameter resolution

d4d0df9

Getting rid of Material caches

2bdae2c

Cleaning up Material.py

0fa5dab

Removing Composite from Material constructor

e329269

Removing material cache crumbs

bec1b2a

Removing propertyRangeUpdated Material state

9a7857b

General material cleanup - sorry

e47def1

Merge branch 'main' into mats_arent_shapes

0ef6c75

Adding code coverage to MOX

74975c7

Fixing linting

f811b3d

john-science added architecture Issues related to big picture system architecture feature request Smaller user request labels Jan 4, 2023

john-science requested a review from ntouran January 4, 2023 18:00

john-science linked an issue Jan 4, 2023 that may be closed by this pull request

Make Materials stateless and remove Composite as their parent #1007

Closed

john-science added cleanup Code/comment cleanup: Low Priority and removed feature request Smaller user request labels Jan 4, 2023

Adding a release note

a856058

ntouran requested changes Jan 5, 2023

View reviewed changes

Removing Material.p.atomFracDenom param

d7ecd50

john-science marked this pull request as draft January 9, 2023 20:24

john-science added 7 commits January 9, 2023 13:21

Removing massFracNorm Material param

d4aed3a

Removing Material.p.thFrac

59dfe3f

Moving refTempK from Parameter to constant

92896dd

Removing theoreticalDensityFrac Parameter from Material

2abf566

Moving theoreticalDensityFrac away from being a Parameter

1095a8e

Moving fuel mat custom isotopics out of Parameters

9bfd323

Removing Fuel Material parameters

467f5f0

john-science added 8 commits January 11, 2023 07:24

Moving uFrac, puFrac, and zrFrac into FuelMaterial

54733d4

Moving mat.p.massFrac to mat.massFrac

2dfd839

Adding Material cache back in

35c12a1

Fixing Material.duplicate() and adding tests for it

87eda3a

Handle FuelMaterial.duplicate() complexity

4f5a333

Responding to Nick's docstring comments

72f5bf2

Fixing issue with MPI materials

ad92d12

Fixing whitespace error in MPI Composite code

c370b79

john-science marked this pull request as ready for review January 12, 2023 14:56

Improving mat cache test

dae0b96

john-science requested a review from ntouran January 12, 2023 16:52

ntouran approved these changes Jan 12, 2023

View reviewed changes

john-science added 11 commits January 12, 2023 12:53

Merge branch 'main' into materials_arent_geometrical

f7c41e1

Adding comment to getNuclides

3f6c102

Adding back in removed cache line

5329777

We no longer need to sync Params on Materials

2d2cdda

Fixing MPI bug with Materials

92594f5

Moving material check to a more general spot

907c11c

Merge branch 'main' into mats_arent_shapes

32e7eb2

Fixing broken unit test

3df3e86

Fixing release doc reference

b817599

Merge branch 'main' into mats_arent_shapes

7b01023

Merge branch 'main' into mats_arent_shapes

ed6d0d7

john-science merged commit 4699c86 into main Jan 24, 2023

john-science deleted the mats_arent_shapes branch January 24, 2023 18:36

keckler mentioned this pull request Jan 24, 2023

TD_frac updates and consistency #1114

Closed

7 tasks

onufer mentioned this pull request Aug 13, 2024

pu frac storage update #1819

Closed

7 tasks

keckler mentioned this pull request Sep 3, 2024

Write and read material theoretical density to/from database #1852

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring Material to not subclass Composite #1062

Refactoring Material to not subclass Composite #1062

john-science commented Jan 4, 2023 •

edited

Loading

john-science commented Jan 4, 2023

drewj-usnctech commented Jan 4, 2023

ntouran Jan 5, 2023

john-science Jan 5, 2023

ntouran Jan 6, 2023

john-science Jan 9, 2023 •

edited

Loading

john-science Jan 11, 2023 •

edited

Loading

john-science Jan 11, 2023

ntouran Jan 12, 2023

ntouran Jan 5, 2023

john-science Jan 5, 2023

john-science Jan 11, 2023

john-science Jan 11, 2023

drewj-usnctech commented Jan 12, 2023

ntouran left a comment

ntouran Jan 12, 2023

ntouran Jan 12, 2023

Refactoring Material to not subclass Composite #1062

Refactoring Material to not subclass Composite #1062

Conversation

john-science commented Jan 4, 2023 • edited Loading

Description

Checklist

john-science commented Jan 4, 2023

drewj-usnctech commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-science Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

john-science Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewj-usnctech commented Jan 12, 2023

ntouran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-science commented Jan 4, 2023 •

edited

Loading

john-science Jan 9, 2023 •

edited

Loading

john-science Jan 11, 2023 •

edited

Loading