Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom densities #1745

Merged
merged 23 commits into from
Jun 27, 2024
Merged

Custom densities #1745

merged 23 commits into from
Jun 27, 2024

Conversation

bsculac
Copy link
Contributor

@bsculac bsculac commented Jun 21, 2024

What is the change?

The ability to specify custom densities for non-custom materials is added in this PR.

Why is the change being made?

Previously, only materials designated "Custom" in the blueprints file were allowed to have custom densities. This change allows perturbation of the component density as the component is generated. This is specifically done to prevent modification of the base material properties, which could have unintended downstream effects.

This PR attempts to resolve #1272. The use case is for adjustment of library materials to account for changes in density during fabrication (e.g. porosity) without having to use workarounds like smear density adjustments.


Checklist

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

@bsculac bsculac requested review from albeanth and onufer June 21, 2024 19:34
mat = materials.resolveMaterialClassByName(self.material)()
if not isinstance(mat, materials.Custom):
densityRatio = (
blueprint.customIsotopics[self.isotopics].density
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we always have a density? i thought some custom isotopic input options maybe didnt specify density. maybe they all eventually resolve it though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the custom isotopics entry can be specified without a density, which is why line 238 checks for it. But if customIsotopics exists then it's attributes are initialized to None

@keckler
Copy link
Member

keckler commented Jun 21, 2024

At what temperature is the entered density supposed to correspond to?

@keckler
Copy link
Member

keckler commented Jun 21, 2024

This PR should probably update this section of the docs:
https://terrapower.github.io/armi/user/inputs.html#custom-isotopics

@bsculac
Copy link
Contributor Author

bsculac commented Jun 21, 2024

At what temperature is the entered density supposed to correspond to?

The density is set at component construction so it should be the cold dimension unless the blueprint was specified to be at hot dimensions.

@keckler
Copy link
Member

keckler commented Jun 21, 2024

At what temperature is the entered density supposed to correspond to?

The density is set at component construction so it should be the cold dimension unless the blueprint was specified to be at hot dimensions.

I suppose I am asking for this to be outlined somewhere, for instance in the blueprints documentation.

@bsculac bsculac removed the request for review from albeanth June 21, 2024 21:52
@bsculac bsculac marked this pull request as draft June 21, 2024 22:48
@john-science john-science added the feature request Smaller user request label Jun 21, 2024
@john-science
Copy link
Member

I would love a unit test that actually tests the new feature you are building here. Show me the goal in your unit test, and show me the edge cases.

And example edge case might be proving that this PR fixes the bug in #1272.

As always, if that's too much of a lift I can help.

bsculac and others added 6 commits June 24, 2024 13:30
the tutorial input was assigning custom isotopics to a component
made of void material. This was being used to check that warnings
were being logged that the material properties were ignored.
Allowing isotopics to be assigned to Void material is not allowed
anymore and an error (not a warning) is thrown. The testing for
material properties being added to void material have been
implemented elsewhere.
@bsculac bsculac marked this pull request as ready for review June 25, 2024 20:32
Co-authored-by: John Stilley <1831479+john-science@users.noreply.github.com>
@john-science john-science merged commit 1eb34bf into terrapower:main Jun 27, 2024
10 checks passed
@bsculac bsculac deleted the customDensities branch June 28, 2024 15:40

# Check that the density is set correctly on the custom density block,
# and that it is not the same as the original
self.assertAlmostEqual(19.1, fuel2.density())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsculac I am curious about this particular assert. It seems like it might be based on the wrong premise.

fuel2 is uses the uranium isotopic number densities, which has a density of 19.1. But in that component Tinput=25 while Thot=600. So after the assembly is constructed and when we are examining it here, shouldn't the fuel's density be decreased from the value of 19.1 (which corresponds to Tinput)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are asking if the custom density logic should be adjusting to the working temperature, probably. When I wrote this PR, I had misunderstood what Tin in the blueprints was used for, see #1818 (comment) for more details. In short, there is an assumption of hot heights in the blueprints unless the option inputHeightsConsideredHot is False, so in the default case of inputHeightsConsideredHot=True the feature in this PR will assign incorrect densities for components with custom isotopics because this feature assumes the blueprints are for cold dimensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

densities are ignored on custom isotopics when material is provided
4 participants