ARMI Materials - Design and Consistency

[john-science]

Recently, I have been working on a couple materials PRs in ARMI. And the sense I am left with is the Materials in ARMI are not consistent enough. I think a little design work is in order.

Let's gather some ideas for things that would improve ARMI materials for:

• consistency
• discoverability

And let's label each idea by whether or not it changes the ARMI API (whether or not downstream repos will need to change).

[john-science]

As @ntouran mentioned here, a lot of our materials have a method named meltingPoint(), but wouldn't that be more clear as meltingPointK(), or something that makes the scientific unit clear?

armi/armi/materials/lithium.py

Lines 66 to 67 in 20ca687

	def meltingPoint(self):
	return 453.69 # K

After all, not all of our Materials data is in the same units.

[john-science]

OR we could demand that ARMI materials use certain units, and create consistency that way.

[keckler]

It should be enforced that all defined material properties have ranges for those properties.

@john-science just went through the arduous process of implementing these ranges for our existing framework materials in #762 using the newly-created propertyValidTemperature dict on the base Material class. This allows for the defined ranges to be discoverable. But there is nothing to enforce that new materials or new material-property methods added to the framework will utilize this new system.

I think it could be very useful to add a "unit test" to the framework that will inspect the material classes and complain if one of the methods returning a supported material property does not have a corresponding entry in the propertyValidTemperature dict.

That way we don't have to rely on code review to enforce this type of consistency in our material classes, and we can guarantee that all materials defined in the framework will have defined ranges for their properties.

This could "break" downstream projects in the sense that the unit tests would fail if a material class is defined in that project that doesn't include the required property ranges.

[keckler]

A similar idea could be applied to other aspects of the material classes as well, to further enforce consistency and quality.

[john-science]

I agree, more consistency would be better.

If the system could be designed to force the developer of a new Material to use the best practices, that would help. Or, sure, if we could device a clever unit test what would fail unless that consistency existed.

Honestly, I would also like to force that a reference is provided for every Material datum.

From like a "major redesign" perspective, we could just have ARMI load materials from a materials DB or flat text file. Like, it demands a defined CSV for each material, and loads the data from there. That way, we could:

• separate the data from the code
• ensure consistency, in that all materials would have the same structure

[john-science]

Is there a reason we need every Material to be a subclass of Composite?

armi/armi/materials/material.py

Line 43 in 20ca687

class Material(composites.Leaf):

This causes a bunch of linting errors / Python subclassing errors for every material:

armi/materials/siC.py:27:0: W0223: Method '__setitem__' is abstract in class 'Composite' but is not overridden (abstract-method)
armi/materials/siC.py:27:0: W0223: Method 'getMaxArea' is abstract in class 'ArmiObject' but is not overridden (abstract-method)
armi/materials/siC.py:27:0: W0223: Method 'getMaxVolume' is abstract in class 'ArmiObject' but is not overridden (abstract-method)

[keckler]

This particular issue is being addressed in #1062

[alexhjames]

Based on the PRs that @john-science just did, all materials currently will call the check range function to make sure the temperature is in the valid range. All future ARMI materials should be forced to use that same architecture so that there is consistent behavior between all materials for what happens when the temperature is in or outside the valid range for that property.
It would be useful to be able to ask a material what properties it has available to provide. This probably could be inferred from the dictionary of valid ranges but not sure if that's the behavior we want coders to be using or not.

[john-science]

I would really like it if we could "force" a temp range check, yeah. @keckler said something similar to me.

It's not, immediately, apparent to me how to do that. So, there's the technical hurdle right now.

I suppose I could pre-populate the propertyValidTemperature() dictionary with something like:

propertyValidTemperature = { 
     "heat capacity": ((0, 1e12), "K"), 
     "linear expansion": ((0, 1e12), "K"), 
     "density": ((0, 1e12), "K"), 
}

Basically, just zero to infinity Kelvin for all the various things that might be temperature dependent. And then every time someone asks for a "heat capacity" or "density" I could check against the valid range.

That would mean that if we just have "density=15.1", that value might have the full zero-infinity range above, or if the reference says that's only true for 300-1200K, we could modify the valid temp range.

Something like that. I'm not sure of how to make it work yet, but it should be possible.

[john-science]

I think a better design for materials in ARMI would be if we separated out the code from the data. For instance, we would have a big text file or database that defined all the values, curves, temperature bounds, and references. And ARMI would just have code that would go though that data and generate a Material object.

This would allow us to:

• enforce consistency
• expose all the data
• separate code from data

[keckler]

Could be a valid option. I've never liked that we have data in ARMI. Seems like it should be in a separate package that ARMI calls to.

[john-science]

Agreed. At the very least ARMI could come with a tiny data package, and downstream users could load their own much bigger, and more robust data sets.

What we have in ARMI right now doesn't amount to much in the way of fully useful data anyway.

[alexhjames]

I really like this idea of seperating the data out from the code.

[john-science]

Also, @drewejohnson notes in a different discussion, that ARMI materials have two parameters that are not consistently applied:

• refTempK
• refDens

It seems likely that we could remove refTempK, and possible remove or clean up the refDens connection with the density() method.

[john-science]

It's not listed as that, but the documentation on why it existed in the first place is pretty thin.

[john-science]

I really think linearExpansion() is supposed to be the CTE or CLTE. But that doesn't mean we don't have something else duplicating that effort incorrectly, I suppose.

[alexhjames]

I agree with linearExpansion() being the CTE, I was asking about the reference temperature. Since the CTE that is defined is a secant value, it always has a companion reference temperature. For example, if you get a CTE value at 100C, and the reference temperature is 20C, then that CTE value is average thermal expansion value between 20C and 100C or in other words the slope of the secant line on the strain vs temperature curve between 20C and 100C.

As the CTE value is relatively meaningless without the reference temperature, the reference temperature should be an accessible parameter.

[john-science]

I like that!

And, just to be clear, is the linear expansion at this reference temperature always zero?

I ask because, if so, I could back-calculate all the refTempKs for ARMI materials, improve the documentation a bit, and make this official.

[alexhjames]

The overall thermal expansion value at the refTempK would be zero but the secant CTE value itself is technically undefined but most people define it as equal to the tangent CTE at that point. I'm not sure whether the ARMI functions are returning the thermal expansion (alpha*dT) or just the CTE (alpha) value.

I think for most of the materials, the reference temperature is generally around 20C however if I remember correctly, B4C is one of the outliers.

[john-science]

ARMI has three similar methods on various materials that describe linear expansion:

• linearExpansion()
• linearExpansionPercent()
• linearExpansionFactor()

These are inconsistently applied across the ARMI Materials. That is, any individual material might have one or two of these, chosen seemingly at random.

However, it seems like linearExpansionFactor() is already written in terms of linearExpansionPercent(). And it should be possible to write linearExpansionPercent() in terms of linearExpansion(), very generally, right?

If so, then if the method linearExpansion() always returns the (probably temperature dependent) CLTE (coefficient of linear thermal expansion), we should only need ONE of these methods and not all three.

[john-science]

Nick suggests this is not possible.

[alexhjames]

Two of the three should be able to be combined. The linearExpansionPercent gives a thermal expansion value between a temperature and the reference temperature. The linearExpansionFactor gives a thermal expansion value between two temperatures. The linearExpansionFactor could be standardized to be combining two calls to linearExpansionPercent. The linearExpansion function cannot be combined because it is giving the tangent CTE which is only related to secant CTE via integrals (see discussion on the other thread).

[john-science]

I guess linearExpansionFactor() is already just a pass-through method that calls linearExpansionPercent() twice. So, that's not as bad as I thought.

[albeanth]

Bringing a few comments in from #868 to provide some additional thoughts on the armi.materials overhaul.

---

(1/3 -- #868 (comment))

There are several implementations for how reference densities are set. It would be great to have consistency. One idea is to create a constructor for each material. I am certainly open to other ideas, I just advocate for consistency and clarity.

---

(2/3 -- #868 (comment))

Would be useful to have the reference temperature (for the reference density) be a parameter that is stored on the database.

Related is the reference temp for the coefficient of thermal expansion. See @alexhjames comment here.

---

(3/3 -- #868 (comment))

Based on #865, I think there is precedent for removing the temperature dependent correlations in materials. At least within the density method. Keeping the temperature dependent correlations is likely useful and may be worth keeping in some new method -- maybe something like tempDependentDensityCorrelation.

[john-science]

I believe ARMI needs an API change:

• density() needs to be mapped to what is now density3()
• pseudoDensity() needs to be mapped to what is now density()

It can not be obvious to our user base that the normal 3D density is actually density3() right now, and the default density() actually maps to a 2D pseudo-density that only applies to axial/assembly reactors.

I understand this is an API change, and I would probably have to open several downstream repo PRs to accommodate it. But the current state of affairs seems unreasonable.

[sombrereau]

This has caused me errors with "What you see is what you get" problems

[john-science]

I really want to do this.

The only reason I haven't is: it will require a bunch of downstream repo PRs.

And density() having such a generic name means that it will be hard for me to track down ALL the possible use-cases.

Even with that though, I really want to make this change.

[john-science]

I have opened a PR against this issue.

[john-science]

@ntouran just noted that some of the ARMI materials have an internal state, which is an upsetting idea.

So, I will attempt to fully squash that "feature" in the ARMI ecosystem. Which, I think, will make ARMI materials easier to use, and definitely easier to reason about.

[keckler]

One possibility for enforcing some level of consistency would be to use the ABC package to make the Material class an abstract class. Then we could force that all subclasses of it have certain methods defined explicitly. This is kind of what was trying to be done with the implementation of SimpleSolid in #868 (at least in my understanding).

This would make it significantly easier to trace through what methods on specific material classes are actually doing.

[john-science]

I like this idea a lot!

I think this would help us solve many of our current problems in ARMI Materials. Because most of the problems on this page, and that people have given me boil down to "I wish ARMI materials were more consistent".

And maybe more would have to be done with, say, valid temperature range enforcement to really guarantee consistency. And it is just the case that we don't HAVE all the curves we'd want for every material. But I can easily envision using abstract base classes as a method to start building real consistency into ARMI materials.

[john-science]

Bump. This is probably really important.

[keckler]

#762 made very good progress on improving the discoverability of our material property temperature ranges. I would like to suggest that we could take this even further.

One thing stands out to me as being potentially problematic --

In the propertyValidTemperature dict that is used to store the ranges, a property can be named whatever the developer has decided it should be. I have seen a couple instances already where the desired name is somewhat ambiguous. For example, should it be "linear expansion", "linear expansion coefficient". Or one of those with a typo! Not knowing exactly how to query the desired property reduces the discoverability of what is in this dict.

I think this type of ambiguity could be reduced if we made the propertyValidTemperature dict into a bonafide class and the property types an enum.

[john-science]

Not a bad idea.

Likewise, it would help if we made the Material class an abstract class, so we could enforce that "linearExpansion" vs "linearExpansionCoefficient".

My little propertyValidTemperature is fine, but is certainly more of a stop-gap than a real solution.

I think we're narrowing in on a set of ideas to make these materials more consistent though.

[keckler]

Just putting a link to this issue here, as it relates to materials and their consistent implementation: #402

[keckler]

Related: #865