armi icon indicating copy to clipboard operation
armi copied to clipboard

Docstring for `Material.density()` is inconsistent with new axial thermal expansion functionality

Open keckler opened this issue 3 years ago • 1 comments

The docstring for the density() method on the base class speaks completely unawares of the ability for thermal axial expansion: https://github.com/terrapower/armi/blob/0a9b8570b18cbfe477cc4dda113ca97d0e9e8c8a/armi/materials/material.py#L337-L357

We probably need to add some nuance to it so that is still makes sense going forward as the thermal axial expansion becomes more commonly used.

@albeanth @john-science

keckler avatar Jul 14 '22 22:07 keckler

@albeanth Thoughts?

john-science avatar Jul 14 '22 23:07 john-science

We believe this issue was resolved with the density/pseudodensity PR:

https://github.com/terrapower/armi/pull/1163

john-science avatar Jul 20 '23 15:07 john-science

I actually disagree. The docstring on density() is now fine, but the issue was effectively shifted to the docstring for pseudoDensity(): https://github.com/terrapower/armi/blob/5912372a9c037f8b6caf52749ebe6dc28c06b873/armi/materials/material.py#L420-L440

In particular, "The component has been manually expanded axially with the manually entered block hot height."

This sentence is only sometimes true, depending on a user's modeling choice and whether or not they use inputHeightsConsideredHot.

keckler avatar Jul 20 '23 16:07 keckler

Okay, @keckler , what do you want the sentence to read? Mind you, not everyone using ARMI will be using the new Axial Thermal Expansion feature, and we want to keep the NOTE on this to be ~one sentence.

john-science avatar Jul 20 '23 16:07 john-science

My proposal would be:

Warning
-------
The pseudoDensity will not typically agree with the component density since this method only expands in 2 dimensions. 
Depending on your use of `inputHeightsConsideredHot` and the current component's temperature,
the pseudoDensity may be a factor of (1 + dLL) different than the density on the component. 

In the case of Fluids, density and pseudoDensity are the same as density is not driven by linear expansion, but 
rather an explicit density function dependent on Temperature. linearExpansionPercent is zero for a fluid. 

I'd want to make sure that @albeanth is good with this modification though.

keckler avatar Jul 20 '23 17:07 keckler

I'd want to make sure that @albeanth is good with this modification though.

I like it. A minor tweak to consider

Warning
---------
Material.pseudoDensity will not typically agree with Material.density or Component.density 
since this methods only expands in 2 dimensions. Depending on your use of 
`inputHeightsConsideredHot` and Component.temperatureInC, Material.psuedoDensity may be
 a factor of (1+dLL) different than Material.density or Component.density. 

In the case of Fluids, Material.density and Material.pseudoDensity are the same .... 
Material.linearExpansionPercent is zero ....

I think the See Also could be rewritten as the first comment is innacurate and the second no longer exists as written. An idea:

See Also
---------
:py:meth:`armi.materials.material.Material.density`
:py:meth:`armi.reactor.components.component.Component.density`

albeanth avatar Jul 20 '23 17:07 albeanth

Good additions!

keckler avatar Jul 20 '23 17:07 keckler