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

Fix tight coupling on lattice interface for Snapshots #1206

Merged
merged 13 commits into from
Mar 24, 2023

Conversation

albeanth
Copy link
Member

@albeanth albeanth commented Mar 7, 2023

Description

For tight coupling, setting the requirement that cross sections be regenerated only when timeNode == 0 was made with only Standard run types with constant power, flow, and/or temperature in mind. When doing snapshot runs with tight coupling, this assumption becomes invalid and it is desirable to always regenerate cross sections. This PR addresses this and modifies the unit testing for the lattice physics interface to test for this.

The unit tests within the lattice physics interface were also somewhat lacking. So some additional unit tests were added to get the coverage up.

A few slight edits to documentations were also added.


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.

@albeanth albeanth added the bug Something is wrong: Highest Priority label Mar 7, 2023
@albeanth albeanth requested a review from mgjarrett March 7, 2023 21:45
@albeanth
Copy link
Member Author

albeanth commented Mar 7, 2023

@onufer @jakehader a heads up.

@mgjarrett
Copy link
Contributor

We're adding this because we're using snapshots to reload from a given statepoint and then run at a different power/flow/temperature condition from the original case.

For now, we are still only updating XS at time node 0 for standard runs because the assumption is that a standard run will have similar power/flow/temperature conditions at all time nodes. This is not necessarily true. With detailed cycle histories, one could burn a reactor at 100% power / 100% flow for 50 days, then drop to 50% power / 50% flow and burn for another 50 days, then drop to 5% power / 20% flow and burn 10 days, etc. If we were to deviate far from the power/flow ratio = 1.0, we would likely want to update XS.

It feels like this caveat above should be documented somewhere, but I don't know the best place. Could be the docstring for this interactCoupled, or the user docs for tight coupling (or both).

albeanth and others added 2 commits March 7, 2023 14:05
Co-authored-by: Michael Jarrett <jarremic@umich.edu>
Co-authored-by: Michael Jarrett <jarremic@umich.edu>
@keckler
Copy link
Member

keckler commented Mar 7, 2023

What @mgjarrett said about varying power levels in a cycle is incredibly important for the user to understand, and is probably something we need to add better treatment for.

@albeanth
Copy link
Member Author

albeanth commented Mar 7, 2023

For now, we are still only updating XS at time node 0 for standard runs because the assumption is that a standard run will have similar power/flow/temperature conditions at all time nodes. This is not necessarily true. With detailed cycle histories, one could burn a reactor at 100% power / 100% flow for 50 days, then drop to 50% power / 50% flow and burn for another 50 days, then drop to 5% power / 20% flow and burn 10 days, etc. If we were to deviate far from the power/flow ratio = 1.0, we would likely want to update XS.

This is an excellent point. I think it would be a worthy feature request in an issue.

It feels like this caveat above should be documented somewhere, but I don't know the best place. Could be the docstring for this interactCoupled, or the user docs for tight coupling (or both).

@jhader and I attempted to provide high-level justification for the time node == 0 case in the docstring for interactCoupled when the method was introduced. We could probably be more precise by calling out the expectation of consistent hot full power. Ironically, the tight coupling documentation directly contradicts this and shows the cross sections being updated at each time step.

@keckler
Copy link
Member

keckler commented Mar 7, 2023

We can put a warning here: https://terrapower.github.io/armi/user/inputs/settings.html#cycle-history

@albeanth
Copy link
Member Author

albeanth commented Mar 7, 2023

Looks like we're getting a massive drop in coverage for latticePhysicsInterface. I think this is due to simply removing the call to loadTestReactor. So the drop may not actually be all that bad... The unit tests for the latticePhysicsInterface prior to this PR were very bare.

I will investigate, but I think we have two options:

  1. bring back loadTestReactor . Which is a bit unnecessary since we don't need a full reactor for the existing interface tests and gives a bit of false coverage, in my opinion.
  2. Add a bunch of dedicated unit tests for latticePhysicsInterface. That can either be in this PR or a separate PR.

@john-science do you have a preference?

@albeanth albeanth marked this pull request as draft March 7, 2023 22:28
@albeanth
Copy link
Member Author

albeanth commented Mar 8, 2023

Looks like we're getting a massive drop in coverage for latticePhysicsInterface. I think this is due to simply removing the call to loadTestReactor. So the drop may not actually be all that bad... The unit tests for the latticePhysicsInterface prior to this PR were very bare....

I went with option 2. See 9eb5844. Coverage looks to be fixed. Locally, I am seeing coverage for latticeInterface.py on main reporting 48% and this PR upping that to 56%.

@albeanth
Copy link
Member Author

albeanth commented Mar 8, 2023

@mgjarrett @keckler - I added a couple of commits reflecting your input on the clarity of the docstring and adding a note to the detailed cycle history docs.
af765f4
78547da

@albeanth albeanth added documentation Improvements or additions to documentation testing Related to tests labels Mar 8, 2023
@albeanth albeanth marked this pull request as ready for review March 8, 2023 22:00
@albeanth albeanth requested a review from mgjarrett March 14, 2023 17:35
@albeanth albeanth merged commit db8dc8c into terrapower:main Mar 24, 2023
@albeanth albeanth deleted the TCFix_XSGen_Snapshots branch March 24, 2023 20:56
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 documentation Improvements or additions to documentation testing Related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants