-
Notifications
You must be signed in to change notification settings - Fork 92
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
Changing constant params to Volume Integrated #1659
Conversation
Reactivity coefficient parameters for the Doppler constant were configured with the ParamLocation.AVERAGE location attribute. These parameters are actually ParamLocation.VOLUME_INTEGRATED. The Doppler constant is directly related to the Doppler coefficient in units of pcm/K; the coefficients were already labeled as ParamLocation.VOLUME_INTEGRATED.
What's the status of this PR? Nominally, it looks like you did what you wanted. Are you waiting to add a unit test or something? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On testing: these params are totally unused in ARMI and fall under the "incomplete feature" umbrella imo (i.e., changing them has no affect in ARMI and it doesn't make sense to test them here).
I think they should be pulled internal where they are actually used.
@mgjarrett Can you put a short release note under the API Changes section? Thanks! |
Okay, we have unit tests for things like "make sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
What is the change?
Reactivity coefficient parameters for the Doppler constant were configured with the
ParamLocation.AVERAGE
location attribute. These parameters are actuallyParamLocation.VOLUME_INTEGRATED
, so they are being changed.Why is the change being made?
The Doppler constant for a given block is proportional to the size of the block. If two blocks were to be combined into one, the combined block Doppler constant would be the sum of the two smaller blocks:
This is achieved with
ParamLocation.VOLUME_INTEGRATED
, notPararmLocation.AVERAGE
.The Doppler constant is directly related to the Doppler coefficient in units of pcm/K; the coefficients were already labeled as
ParamLocation.VOLUME_INTEGRATED
.Checklist
doc
folder.pyproject.toml
.