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.

Sign up for GitHub

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

Jump to bottom

Update stationary block methods #665

Merged
merged 21 commits into from
Aug 9, 2022
Merged

Update stationary block methods #665

merged 21 commits into from
Aug 9, 2022

Conversation

HunterPSmith
Copy link
Contributor

@HunterPSmith HunterPSmith commented May 9, 2022 •
edited by john-science
Loading

Description

Updated the way that ARMI handles stationary blocks. ARMI now has a global setting called stationaryBlockFlags which can be set by the user to identify flags for stationary blocks. By default, GRID_PLATE has a stationary flag, unless set by user in the input file. When a reactor object is initialized, a reactor attribute called stationaryBlockFlagsList is generated which contains a list of the flags in the reactor that are considered stationary. ARMI now operates on stationaryBlockFlagsList in order to control stationary blocks.

Added testing to satisfy requirement MDN-FMM-004 of the FMM SRSD.

This PR will require the MCNP plugin to be updated.

Checklist

  • The code is understandable and maintainable to people beyond the author.
  • Tests have been added/updated to verify that the new or changed code works.
  • There is no commented out code in this PR.
  • The commit message follows good practices.
  • All docstrings are still up-to-date with these changes.

If user exposed functionality was added/changed:

  • Documentation added/updated in the doc folder.
  • New or updated dependencies have been added to setup.py.

HunterPSmith and others added 8 commits May 3, 2022 10:00
@john-science john-science added the feature request Smaller user request label May 9, 2022
@john-science
Copy link
Member

I love it!

I have made two comments/requests. But they are pretty minor.

ntouran
ntouran requested changes May 10, 2022
View reviewed changes
Copy link
Member

@ntouran ntouran left a comment

Choose a reason for hiding this comment

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

Seems like a good change.

  • This is a breaking change and will break all previous inputs that used the stationaryBlocks setting. This needs to be documented in the documentation and in the release notes.
  • Would you consider adding a migration so that users with the deprecated setting can proceed easily? Or at least provide a meaningful error message telling them how to proceed.

armi/physics/fuelCycle/fuelHandlers.py Outdated Show resolved Hide resolved
john-science and others added 3 commits May 16, 2022 20:52
@john-science
Merge branch 'master' into master
1268be3
@HunterPSmith
Add testing for dischargeSwap (#8)
696bbc7
@HunterPSmith
Fmm sdid (#9)
8805011 
* Add testing for dischargeSwap

* Add error for old stationary block setting
@HunterPSmith
Copy link
Contributor Author

Seems like a good change.

  • This is a breaking change and will break all previous inputs that used the stationaryBlocks setting. This needs to be documented in the documentation and in the release notes.
  • Would you consider adding a migration so that users with the deprecated setting can proceed easily? Or at least provide a meaningful error message telling them how to proceed.

I am not familiar which specific documentation this would go in. I assume that the release notes are here: /nala/framework/doc/release/0.2.rst ?

@john-science
Copy link
Member

I am not familiar which specific documentation this would go in. I assume that the release notes are here: /nala/framework/doc/release/0.2.rst ?

That's it! A quick one-sentence explanation of the breaking change.

@HunterPSmith HunterPSmith requested a review from ntouran May 30, 2022 16:26
jakehader
jakehader requested changes Jun 9, 2022
View reviewed changes
armi/settings/fwSettings/globalSettings.py Outdated Show resolved Hide resolved
armi/reactor/reactors.py Outdated Show resolved Hide resolved
armi/physics/fuelCycle/fuelHandlers.py Outdated Show resolved Hide resolved
armi/physics/fuelCycle/fuelHandlers.py Outdated Show resolved Hide resolved
armi/physics/fuelCycle/fuelHandlers.py Outdated Show resolved Hide resolved
@HunterPSmith
Remove stationaryBlocks setting (#14)
553a165
@HunterPSmith
Remove _swapFluxParam method. Update transferStationaryBlocks method. (…
4113bda 
…#15)

* Remove ``stationaryBlocks`` setting

* Remove ``_swapFluxParam`` method. Update ``_transferStationaryBlocks`` method for clarity.
jakehader
jakehader approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@jakehader jakehader left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@HunterPSmith, very sorry for the long review time on this. Can you work with @opotowsky to make sure we understand the full scope of the impact to internal plugins offline and testing before we land this?

opotowsky
opotowsky requested changes Aug 5, 2022
View reviewed changes
Copy link
Member

@opotowsky opotowsky left a comment

Choose a reason for hiding this comment

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

Just a quick cleanup request:

This line in armi/reactor/tests/test_reactors.py should be deleted:

image

@opotowsky
Copy link
Member

@jakehader @HunterPSmith Internal plugins and testing are a go. This PR is good to merge from that perspective.

@jakehader
Copy link
Member

Great, thanks @opotowsky! @HunterPSmith, can you address the last comments here and we will merge?

@HunterPSmith
Copy link
Contributor Author

Thank you both! Should be good to go now

@john-science john-science merged commit 9a510ca into terrapower:main Aug 9, 2022
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
@HunterPSmith @scottyak
Updating stationary block methods (terrapower#665)
ceefc18 
Updated the way that ARMI handles stationary blocks. ARMI now has a global setting called stationaryBlockFlags which can be set by the user to identify flags for stationary blocks. By default, GRID_PLATE has a stationary flag, unless set by user in the input file. When a reactor object is initialized, a reactor attribute called stationaryBlockFlagsList is generated which contains a list of the flags in the reactor that are considered stationary. ARMI now operates on stationaryBlockFlagsList in order to control stationary blocks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keckler keckler keckler left review comments

@jakehader jakehader jakehader approved these changes

@opotowsky opotowsky opotowsky approved these changes

@john-science john-science john-science approved these changes

@ntouran ntouran Awaiting requested review from ntouran

Assignees
No one assigned
Labels
feature request Smaller user request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@HunterPSmith @john-science @opotowsky @jakehader @keckler @ntouran