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

Fixing Component density to agree with 3D density #860

Merged
merged 7 commits into from Aug 26, 2022
Merged

Fixing Component density to agree with 3D density #860

merged 7 commits into from Aug 26, 2022

Conversation

onufer
Copy link
Contributor

@onufer onufer commented Aug 25, 2022

Description

Resolves #820 so component agrees exactly with 3D density. Removed some testing work arounds.

Checklist

  • This PR has only one purpose or idea.
  • Tests have been added/updated to verify that the new/changed code works.
  • The release notes (location doc/release/0.X.rst) 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.

@onufer onufer requested a review from jakehader August 25, 2022 16:54
@opotowsky opotowsky linked an issue Aug 25, 2022 that may be closed by this pull request
@opotowsky opotowsky added the bug Something is wrong: Highest Priority label Aug 25, 2022
@onufer onufer requested a review from ntouran August 25, 2022 17:00
@onufer
Copy link
Contributor Author

onufer commented Aug 25, 2022

@mgjarrett, please feel free to add comments

@@ -130,7 +130,7 @@ def linearExpansionPercent(self, Tk=None, Tc=None):
Tk = getTk(Tc, Tk)
self.checkPropertyTempRange("linear expansion percent", Tk)

if Tk >= 291.62 and Tk < 1137:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This reverted a change that was needed to change the blueprints which also were not needed.

# axialExpansionFactor = 1.0 + self.material.linearExpansionFactor(
# self.temperatureInC, self.inputTemperatureInC
# )
self.changeNDensByFactor(1.0 / self.getThermalExpansionFactor())
Copy link
Contributor

Choose a reason for hiding this comment

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

The else seems to only be invoked in tests, as far as I can tell. Could we just remove the initialColdMaterialExpansion argument, and then rewrite and/or remove those tests as appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is invoked in a TP submodule. I would also expect it could also be useful for axial expansion, although it is not used now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would moving the code from the if statement into applyMaterialMassFracsToNumberDensities be acceptable, and then the if statement would be eliminated but we would still have a useful hot height density reduction changer.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the code is used then I don't have a problem with it being here. I was just proposing removing the code if in fact it was not being used anymore.

@onufer
Copy link
Contributor Author

onufer commented Aug 25, 2022

wait for @ntouran to have a chance to provide feedback to merge

@john-science john-science changed the title Fix820 Fixing Component density to agree with 3D density Aug 25, 2022
@jakehader jakehader removed their request for review August 26, 2022 03:23
@jakehader
Copy link
Member

Taking myself off review @onufer. I think that @mgjarrett and @ntouran can have this covered.

@onufer onufer merged commit ec73cb7 into main Aug 26, 2022
@onufer onufer deleted the fix820 branch August 26, 2022 20:10
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
* Added a couple component expansion methods:  adjustDensityForHeightExpansion, getHeightFactor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong: Highest Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component density does not exactly agree with 3D density
4 participants