Fixing the affect of custom isotopics on material density #1822

jakehader · 2024-08-14T15:20:05Z

What is the change?

This fixes some problems that were found with #1745. Those issues are described in #1818 .

Why is the change being made?

We are fixing a bug that made it into the codebase.

Checklist

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

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

The release notes have been updated if necessary.
The documentation is still up-to-date in the doc folder.
The dependencies are still up-to-date in pyproject.toml.

john-science · 2024-08-14T16:00:33Z

@jakehader Just a reminder of the ARMI PR policy:

armi/doc/developer/tooling.rst

Lines 45 to 49 in bd0b43a

    
           Don't open until it is ready 
        
           ---------------------------- 
        
           .. important :: 
        
               Wait until your PR is complete to open it.

We are all extremely busy, and if a PR gets opened before it is ready I will get an email:

every single time you commit, and the CI passes
for each and every time a single CI job fails

I love the help, and the interesting in fixing the feature. But in the future, please wait until the feature is ready for final review before opening a PR.

armi/reactor/blueprints/componentBlueprint.py

jakehader · 2024-08-15T02:05:56Z

@jakehader Just a reminder of the ARMI PR policy:

armi/doc/developer/tooling.rst

Lines 45 to 49 in bd0b43a

Don't open until it is ready

----------------------------

.. important ::

Wait until your PR is complete to open it.

We are all extremely busy, and if a PR gets opened before it is ready I will get an email:

every single time you commit, and the CI passes

for each and every time a single CI job fails

I love the help, and the interesting in fixing the feature. But in the future, please wait until the feature is ready for final review before opening a PR.

Thanks for the reminder @john-science - I put the PR here but forgot to make it a draft. This was just intended to provide some ideas for how to resolve the ticket and to put something out there to be stress tested by @keckler and others.

There are failing unit tests because I changed the construct arguments so that needs to be resolved still (probably a poor design to require case settings to build a component). Also, we will need to add more unit tests around custom Isotopics (same comment on the ticket).

keckler · 2024-08-15T14:05:31Z

I ran some tests on my FFTF model to see if this branch works. A couple points:

The keff in my model increased by nearly 5000 pcm to a value that is very incorrect.
The predicted isothermal temperature reactivity coefficient looks correct.
The fuel density when the model is initialized to Thot is way too high (~10%).
The change in density between different Thot values appears to be roughly correct, and the number of atoms is being correctly conserved.

So it seems like this change is half working. Somehow the initialized model is getting the densities very incorrect, though.

jakehader · 2024-10-30T07:24:42Z

Was this fixed elsewhere @keckler?

keckler · 2024-10-30T13:10:46Z

Nope, still broken

…ad of the whole cs

armi/reactor/blueprints/tests/test_customIsotopics.py

keckler · 2024-12-01T23:43:33Z

armi/reactor/blueprints/isotopicOptions.py

@@ -394,6 +394,7 @@ def apply(self, material):
        material.massFrac = dict(self.massFracs)
        if self.density is not None:
            if not isinstance(material, materials.Custom):
+                # TODO I think this warning should go away?


I would like a second set of eyes on the runLog statement below. I am 99% sure that that print should be removed completely, because we are now allowing for non-Custom materials to have density specified.

The runlog appears to still be correct and applicable, as the density is not applied to material in this function if material is not a custom material per line 396. I don't recognize anywhere in the changes that the density of a library material is being set, am I missing it?

Related: I believe this was intended to explicitly call out that setting a custom density for a library material using custom isotopics is not allowed. Presumably a library material would have known, vetted properties that should not be overwritten by users in a passive way. From recollection, using custom isotopics on a library material allows the density to be set on a component specifically using the custom isotopics, but using custom isotopics to set the density for the reference material is no bueno.

I think you're right.

The thing that keeps tripping me up is the last sentence: "Only custom materials may have a density specified."

I feel like that can be reworded to make the intention more clear. Right now I read that and think that its saying "the density that you specified is doing nothing".

I'll try to rejigger a bit.

Yeah, I vaguely remember struggling how to phrase this without being very wordy. Some ideas:

"Only custom materials may have a density specified using custom isotopics, though components using custom materials may have their densities changed."

"The reference density of materials in the materials library will not be changed by custom isotopics, but components with custom isotopics using a library material will use the density implied by the custom isotopics."

Alright, I updated the wording. Thanks for helping here.

keckler · 2024-12-02T22:27:22Z

@john-science I believe that this is ready for your eyes.

In addition to the updated unit tests, I did quite a bit of offline testing with my FFTF model to convince myself that this is working properly.

john-science · 2024-12-02T23:04:44Z

Random thought: I think this PR is big enough for a release note in doc/release/0.5.rst.

john-science · 2024-12-03T00:03:40Z

armi/reactor/blueprints/componentBlueprint.py

@@ -186,7 +186,7 @@ def shape(self, shape):
    mergeWith = yamlize.Attribute(type=str, default=None)
    area = yamlize.Attribute(type=float, default=None)

-    def construct(self, blueprint, matMods):
+    def construct(self, blueprint, matMods, inputHeightsConsideredHot):


Okay, so the variable name here is strange. Because "input heights" makes sense if this Component is part of an Assembly in some way, sure.

But Components are a very general idea of the leaf of the data model. They don't have to be in a pin-type reactor.

What do you think?

Also, maybe this new parameter should have a default value? What do you think?

This parameter and its name are directly copied from the case setting of the same name. I tried to reflect that intention in the docstring, but maybe I could be more clear there.

When Jake put up the draft PR, he was originally passing an entire case settings object down to this level. I did away with that because I find it frustrating when I have to whip up an entire cs just for a single boolean.

So I made the name for this parameter exactly the same as the case setting whose value should be passed down here. I thought that made things simpler, but if you think it'd be more clear either (1) with a different name, or (2) if we passed the entire cs, then I am open to those options.

With respect to a default value...

I'm not sure how I feel there.

On the one hand, we could potentially make the default be the same as the default for the inputHeightsConsideredHot case setting (True). On the other hand, I really don't like that because the default on that case setting is the opposite of our typical workflow at this point in time (i.e. having axial expansion turned on).

I think my ideal solution would be if there were a way to not have to pass the value via method calls. But the only way I could conceive to do that is to store a cs on the ComponentBlueprint, and that is something I really don't want to do.

Maybe you see a clean way to get around all this?

It is definitely better to pass one value instead of the entire settings object. For sure. Nice.

I suppose "input dimensions considered hot" would have been a more accurate name for that setting. But the people who created it were definitely thinking of pin-type reactors.

doc/release/0.5.rst

Co-authored-by: John Stilley <[email protected]>

…er/armi into customIsotopicsStrawman

Strawman fix for the custom isotopics for testing.

7b258d4

jakehader mentioned this pull request Aug 14, 2024

Use of custom isotopics with thermal expansion turned on does not correctly preserve mass #1818

Closed

john-science requested review from keckler and john-science August 14, 2024 15:57

jakehader marked this pull request as draft August 14, 2024 17:04

keckler linked an issue Aug 14, 2024 that may be closed by this pull request

Use of custom isotopics with thermal expansion turned on does not correctly preserve mass #1818

Closed

keckler reviewed Aug 14, 2024

View reviewed changes

armi/reactor/blueprints/componentBlueprint.py Outdated Show resolved Hide resolved

keckler added 11 commits November 30, 2024 13:44

Merge branch 'main' into customIsotopicsStrawman

d7f2f91

Remove print statements and fix black formatting

1290e14

Fix improperly resolved merge conflict

fd831ca

Refactor some runLog prints

895e61b

Add some TODOs for reviewers to think about

db06fd4

Pass in the correct recursive args)

55d8c13

Fix test

3965082

Fix unit tests that were broken from the start

d978533

Pass just the boolean inputHeightsConsideredHot down the chain, inste…

6a06967

…ad of the whole cs

Clean things up

befd6a7

Make it an error when the density for a material is overspecified

d59e8ab

keckler reviewed Dec 1, 2024

View reviewed changes

armi/reactor/blueprints/tests/test_customIsotopics.py Show resolved Hide resolved

keckler reviewed Dec 1, 2024

View reviewed changes

armi/reactor/blueprints/tests/test_customIsotopics.py Show resolved Hide resolved

keckler reviewed Dec 1, 2024

View reviewed changes

keckler added 2 commits December 1, 2024 17:54

Fix runLog statement

c1270df

Fix test that changed due to a different runLog statement

53ff4d3

keckler changed the title ~~Strawman fix for the custom isotopics for testing.~~ Fix the use of custom isotopics combined with a material class Dec 2, 2024

Update runLog statement for clarity

4fc8012

keckler added 2 commits December 2, 2024 16:24

Remove TODO

2cfe89b

Update test for runLog wording change

e483fef

keckler marked this pull request as ready for review December 2, 2024 22:27

terrapower deleted a comment from keckler Dec 2, 2024

john-science changed the title ~~Fix the use of custom isotopics combined with a material class~~ Fixing the affect of custom isotopics on material density Dec 3, 2024

john-science reviewed Dec 3, 2024

View reviewed changes

Add release notes

7da315a

john-science reviewed Dec 3, 2024

View reviewed changes

doc/release/0.5.rst Outdated Show resolved Hide resolved

Update doc/release/0.5.rst

c33c893

Co-authored-by: John Stilley <[email protected]>

john-science approved these changes Dec 3, 2024

View reviewed changes

keckler added 2 commits December 3, 2024 11:28

Merge branch 'main' into customIsotopicsStrawman

2f27761

Merge branch 'customIsotopicsStrawman' of https://github.com/terrapow…

d85cc1e

…er/armi into customIsotopicsStrawman

john-science merged commit 68268c1 into main Dec 3, 2024
22 checks passed

john-science deleted the customIsotopicsStrawman branch December 3, 2024 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing the affect of custom isotopics on material density #1822

Fixing the affect of custom isotopics on material density #1822

jakehader commented Aug 14, 2024 •

edited by keckler

Loading

john-science commented Aug 14, 2024

jakehader commented Aug 15, 2024

keckler commented Aug 15, 2024

jakehader commented Oct 30, 2024

keckler commented Oct 30, 2024

keckler Dec 1, 2024

keckler Dec 2, 2024

bsculac Dec 2, 2024

keckler Dec 2, 2024

bsculac Dec 2, 2024

keckler Dec 2, 2024

keckler commented Dec 2, 2024

john-science commented Dec 2, 2024

john-science Dec 3, 2024 •

edited

Loading

john-science Dec 3, 2024

keckler Dec 3, 2024 •

edited

Loading

keckler Dec 3, 2024

john-science Dec 3, 2024

Fixing the affect of custom isotopics on material density #1822

Fixing the affect of custom isotopics on material density #1822

Conversation

jakehader commented Aug 14, 2024 • edited by keckler Loading

What is the change?

Why is the change being made?

Checklist

john-science commented Aug 14, 2024

jakehader commented Aug 15, 2024

keckler commented Aug 15, 2024

jakehader commented Oct 30, 2024

keckler commented Oct 30, 2024

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

keckler commented Dec 2, 2024

john-science commented Dec 2, 2024

john-science Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keckler Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakehader commented Aug 14, 2024 •

edited by keckler

Loading

john-science Dec 3, 2024 •

edited

Loading

keckler Dec 3, 2024 •

edited

Loading