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

Changing constant params to Volume Integrated #1659

Merged
merged 5 commits into from
Jun 10, 2024

Conversation

mgjarrett
Copy link
Contributor

@mgjarrett mgjarrett commented Mar 7, 2024

What is the change?

Reactivity coefficient parameters for the Doppler constant were configured with the ParamLocation.AVERAGE location attribute. These parameters are actually ParamLocation.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:

b.p.rxFuelDopplerConstant = b1.p.rxFuelDopplerConstant + b2.p.rxFuelDopplerConstant

This is achieved with ParamLocation.VOLUME_INTEGRATED, not PararmLocation.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

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

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.
@john-science john-science self-requested a review March 7, 2024 23:23
@john-science john-science added the enhancement New feature or request label Mar 7, 2024
@mgjarrett mgjarrett requested a review from albeanth March 25, 2024 18:45
@mgjarrett mgjarrett self-assigned this Mar 25, 2024
@john-science
Copy link
Member

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?

@albeanth albeanth requested review from keckler and removed request for albeanth April 3, 2024 16:15
@albeanth albeanth requested review from albeanth and removed request for keckler April 30, 2024 22:40
Copy link
Member

@albeanth albeanth left a 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.

@john-science john-science marked this pull request as ready for review June 7, 2024 20:46
@john-science john-science changed the title Change the properties of Doppler constant params Changing constant params to Volume Integrated Jun 7, 2024
@john-science
Copy link
Member

john-science commented Jun 7, 2024

@mgjarrett Can you put a short release note under the API Changes section? Thanks!

@john-science
Copy link
Member

john-science commented Jun 7, 2024

It's odd this change doesn't require any unit tests changes, right?

Okay, we have unit tests for things like "make sure VOLUME_INTEGRATED works", but not testing it for each Parameter. I guess that's the best idea I can come up with right now.

Copy link
Member

@john-science john-science left a comment

Choose a reason for hiding this comment

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

Great!

@mgjarrett mgjarrett merged commit d8c4331 into terrapower:main Jun 10, 2024
12 checks passed
@mgjarrett mgjarrett deleted the changeRxCoeffparams branch June 10, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants