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

Specifying both Tk and Tc is an error #859

Merged
merged 2 commits into from
Aug 25, 2022

Conversation

dlangewisch
Copy link
Contributor

Description

This PR makes it an error to specify both Tk and Tc parameters to material properties routines. This is intended to prevent unexpected behavior such as sodium.density(200, 300) being a valid call (the second argument is silently ignored). Unit tests have also been added to test the getTk, getTc, and getTf functions in the utils.units module.


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.

A ValueError is now raised if getTc, getTk, or getTf are not given
exactly one argument. This prevents unexpected behavior such as
`sodium.density(Tc=100, Tk=100)` being a valid call.
Calling Sodium.thermalConductivity with the Tc argument (and no Tk
argument) resulted in an error.
@jakehader jakehader merged commit 7bd160d into terrapower:main Aug 25, 2022
@dlangewisch dlangewisch deleted the only_one_Tk_or_Tc branch August 25, 2022 15:46
@john-science john-science added the bug Something is wrong: Highest Priority label Aug 25, 2022
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
* A ValueError is now raised if getTc, getTk, or getTf are not given
exactly one argument. This prevents unexpected behavior such as
`sodium.density(Tc=100, Tk=100)` being a valid call.

* Fixed bug in Sodium.thermalConductivity (closes terrapower#858)

* Calling Sodium.thermalConductivity with the Tc argument (and no Tk
argument) resulted in an error.
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.

None yet

3 participants