Conserve component density #846

mgjarrett · 2022-08-19T21:29:58Z

Description

The axialExpansionChanger was implemented to conserve the initial mass of each component in a block with axial expansion determined by the target component, using the funciton _conserveComponentMass. Since different components are made of different materials that thermally expand at different rates, this necessarily means that the densities have to be adjusted artificially for the mass to be conserved over the block for each individual component.

After testing and discussing the methodology, it has been decided that it is preferred to conserve the realistic density of the material rather than conserving mass within a block. Effectively, we are allowing mass to move between blocks.

The Tinput values in the test reactor for detailedAxialExpansion were updated because the unit test to ensure density conservation requires that the Tinput is equal to the material reference temperature for calculating thermal expansion factors.

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

Edit the Tinput for HT9 and UZr to the reference temperatures to make calculation of density3 consistent with nuclide number densities for testing.

mgjarrett · 2022-08-20T00:32:03Z

@albeanth I took a quick glance at the docs/release notes and didn't see anything that needed to be updated, but you would probably know better than me.

armi/reactor/converters/axialExpansionChanger.py

jakehader · 2022-08-21T19:49:31Z

armi/tests/detailedAxialExpansion/refSmallReactorBase.yaml

            Thot: 470.0
            id: 1.0
            mult: 169.0
            od: 1.09
        wire: &component_plenum_wire
            shape: Helix
            material: HT9
-            Tinput: 25.0
+            Tinput: 27.548


Can you explain in the PR description why these are changing? I assume that this is because we have inputHeightsConsideredHot set to True for the tests? Not clear though.

The reference temperature of HT9 for thermal expansion is 27.548C. The test to check tohat density is conserved only works if Tinput is equal to the reference temperature for thermal expanison.

In line with @jakehader 's comment:

@mgjarrett - for our future selves I think it would be good to leave a comment in this PR on precisely why you/we are making the changes to these temperatures and to armi/materials/zr.py (especially the latter).

I could easily see us coming back a few months later and being totally stumped as to why these temperatures were all changed.

@albeanth are you looking for a comment directly in the yaml file? Or just include it in the commit message?

The commit message would be fine, but I was thinking about just a general comment in this PR. That way if someone in the future does a git blame and comes back to this PR it will be (somewhat) front and center.

I suppose a comment in the yaml would be ok too - maybe at the very top right off the bat? Not sure where it's best to include it. I'll leave it to your discretion.

jakehader

See comments.

albeanth · 2022-08-22T14:47:14Z

@mgjarrett Haven't had a chance to review this yet. But am noticing that the coverage dropped pretty drastically. Especially in axialExpansionChanger.py. Can you confirm that you are getting the same coverage drop on your local machine?

mgjarrett · 2022-08-22T16:24:44Z

@albeanth I'm not sure why it had coverage decreasing significantly. It's now at -0.003%, which seems better. But when I look at the coveralls report, it looks messed up. Says that 246 existing lines are uncovered and 17 new are covered, which is not consistent with -0.003%.

mgjarrett · 2022-08-22T16:50:40Z

What's confusing is that the coverage rose in commit ec29050

The next 3 commits are just changing function and variable names, but no functional changes have been implemented at all. Now it says coverage decreased. One of those commits broke a unit test, which might have confused coveralls.

mgjarrett · 2022-08-22T18:03:37Z

@jakehader I've addressed your comments. I'm trying to understand what happened to the coverage but I'm having trouble understanding what happened from the coveralls report. It does appear that all of the new lines are covered.

mgjarrett · 2022-08-22T18:05:30Z

armi/tests/detailedAxialExpansion/refSmallReactorBase.yaml

@@ -547,7 +547,7 @@ assemblies:
        height: *highOffset_height
        axial mesh points: *standard_axial_mesh_points
        xs types: *igniter_fuel_xs_types
-    lta fuel:
+    lead test fuel:


Need these to have Flags.TEST so they can be skipped in the density conservation unit test, because the cladding has liner merged at different temperatures.

armi/reactor/converters/tests/test_axialExpansionChanger.py

albeanth

Approving, but would love to see a comment addressing this. Thanks @mgjarrett !
#846 (comment)

jakehader · 2022-08-22T23:42:32Z

Okay, after talking with @mgjarrett I think I understand the scope of this change. Basically, we are demonstrating that if the component's input temperature is equal to its material's refTemp then thermal expansion will be correctly treated. Otherwise, if these inputs are not equal then the material.density3() and c.getMassDensity() will not be equal and therefore be off by a factor of the thermal expansion coefficients between the material at the reference temperature and the input temperatures. Please correct my @mgjarrett if this is wrong.

If the above is correct, then I suggest adding a warning to the user that the component input reference does not align with the material's refTemp, which can probably be added here: https://github.com/terrapower/armi/blob/main/armi/reactor/components/component.py#L318-L328

I am not sure, but this will probably break downstream unit tests that we will have to fix. @opotowsky, would you mind pulling this in and running all internal plugin unit tests and the integration tests?

mgjarrett · 2022-08-23T00:38:49Z

@jakehader that summary is correct. I've added a warning when the absolute value of the thermal expansion factor at Tinput is greater than 1e-6.

I am firing off some downstream tests right now. I expect the impacts to be minimal but we will see.

armi/reactor/components/component.py

albeanth · 2022-08-23T14:27:09Z

@albeanth I took a quick glance at the docs/release notes and didn't see anything that needed to be updated, but you would probably know better than me.

To my knowledge, no need to add anything to the docs.

I would add one line to the release notes commenting on the density conservation bug fix.

mgjarrett · 2022-08-23T17:57:51Z

Downstream tests are currently failing; waiting to merge this until we get that resolved

opotowsky · 2022-08-23T21:20:47Z

Downstream tests resolved -- merging!

onufer · 2022-08-24T03:21:27Z

I would expect that after #820 is resolved the odd initial temps in blueprints would not longer be needed

armi/reactor/components/component.py

* Add failing unit test for conserving density. * Fix failing density unit test. * Correct Tinput for fuel. * Loosen test criterion for Zr. * Don't change density of fluid materials. * Fix specified expansion test. Edit the Tinput for HT9 and UZr to the reference temperatures to make calculation of density3 consistent with nuclide number densities for testing. * Update specifyTargetComponent to use solid material. * Change function name and update docstring. * Fix function name. * Update variable name. * Fix inadvertent change in test precision. * Fix reference temp for Zr thermal expansion. * Update error message for density check. * Reset the Zr reference temperature. * Add warning for Tinput != referenceTemp. * Add line in the release notes. * Make Tinput warning a single.

mgjarrett added 8 commits August 19, 2022 14:25

Add failing unit test for conserving density.

ab52a9c

Fix failing density unit test.

9b9647c

Correct Tinput for fuel.

73b8a08

Loosen test criterion for Zr.

ee081c8

Don't change density of fluid materials.

c90155b

Fix specified expansion test.

a671ca8

Edit the Tinput for HT9 and UZr to the reference temperatures to make calculation of density3 consistent with nuclide number densities for testing.

Update specifyTargetComponent to use solid material.

ec29050

Change function name and update docstring.

4eff3e2

mgjarrett mentioned this pull request Aug 20, 2022

Improve Flexibility Of Component Temp Mapping + Bug Fix With Radial NDens Update #845

Merged

7 tasks

Fix function name.

cd55c4e

jakehader requested changes Aug 21, 2022

View reviewed changes

Update variable name.

ed620df

mgjarrett commented Aug 22, 2022

View reviewed changes

mgjarrett added 2 commits August 22, 2022 13:36

Fix inadvertent change in test precision.

8d114bb

Fix reference temp for Zr thermal expansion.

4656386

albeanth requested changes Aug 22, 2022

View reviewed changes

armi/reactor/converters/tests/test_axialExpansionChanger.py Show resolved Hide resolved

mgjarrett added 2 commits August 22, 2022 15:28

Update error message for density check.

f8b2a8e

Reset the Zr reference temperature.

ee83ba0

albeanth approved these changes Aug 22, 2022

View reviewed changes

Add warning for Tinput != referenceTemp.

59ac6b8

jakehader reviewed Aug 23, 2022

View reviewed changes

armi/reactor/components/component.py Show resolved Hide resolved

mgjarrett added 2 commits August 23, 2022 07:38

Add line in the release notes.

d31b532

Make Tinput warning a single.

3f17e4b

jakehader approved these changes Aug 23, 2022

View reviewed changes

opotowsky merged commit de18f60 into terrapower:main Aug 23, 2022

mgjarrett deleted the conserveComponentDensity branch August 23, 2022 22:26

john-science reviewed Aug 25, 2022

View reviewed changes

armi/reactor/components/component.py Show resolved Hide resolved

albeanth mentioned this pull request Sep 15, 2022

Add test for hot and cold mass in inputHeightsConsideredHot albeanth/armi#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conserve component density #846

Conserve component density #846

mgjarrett commented Aug 19, 2022 •

edited

Loading

mgjarrett commented Aug 20, 2022

jakehader Aug 21, 2022

mgjarrett Aug 22, 2022

albeanth Aug 22, 2022

mgjarrett Aug 23, 2022

albeanth Aug 23, 2022

jakehader left a comment

albeanth commented Aug 22, 2022

mgjarrett commented Aug 22, 2022

mgjarrett commented Aug 22, 2022

mgjarrett commented Aug 22, 2022

mgjarrett Aug 22, 2022

albeanth left a comment

jakehader commented Aug 22, 2022

mgjarrett commented Aug 23, 2022

albeanth commented Aug 23, 2022

mgjarrett commented Aug 23, 2022

opotowsky commented Aug 23, 2022

onufer commented Aug 24, 2022

Conserve component density #846

Conserve component density #846

Conversation

mgjarrett commented Aug 19, 2022 • edited Loading

Description

Checklist

mgjarrett commented Aug 20, 2022

jakehader Aug 21, 2022

Choose a reason for hiding this comment

mgjarrett Aug 22, 2022

Choose a reason for hiding this comment

albeanth Aug 22, 2022

Choose a reason for hiding this comment

mgjarrett Aug 23, 2022

Choose a reason for hiding this comment

albeanth Aug 23, 2022

Choose a reason for hiding this comment

jakehader left a comment

Choose a reason for hiding this comment

albeanth commented Aug 22, 2022

mgjarrett commented Aug 22, 2022

mgjarrett commented Aug 22, 2022

mgjarrett commented Aug 22, 2022

mgjarrett Aug 22, 2022

Choose a reason for hiding this comment

albeanth left a comment

Choose a reason for hiding this comment

jakehader commented Aug 22, 2022

mgjarrett commented Aug 23, 2022

albeanth commented Aug 23, 2022

mgjarrett commented Aug 23, 2022

opotowsky commented Aug 23, 2022

onufer commented Aug 24, 2022

mgjarrett commented Aug 19, 2022 •

edited

Loading