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

Expose tight coupling to OperatorSnapshots #1113

Merged
merged 5 commits into from
Jan 24, 2023
Merged

Expose tight coupling to OperatorSnapshots #1113

merged 5 commits into from
Jan 24, 2023

Conversation

albeanth
Copy link
Member

@albeanth albeanth commented Jan 23, 2023 •
edited
Loading

Description

PR #1033 introduced the capability to measure the convergence of tight coupling and removed the looseCoupling setting. However, its exposure to the snapshot operator was missed. This PR serves to resolve that.

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 Jan 23, 2023
opotowsky
opotowsky approved these changes Jan 23, 2023
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.

I haven't been involved in this feature add, but this PR looks good to me!

john-science
john-science approved these changes Jan 23, 2023
View reviewed changes
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.

It's a small change. And I see its value.

I'd like the change more if it broke a test. Or we added a test to cover it.

@albeanth
Copy link
Member Author

I'd like the change more if it broke a test. Or we added a test to cover it.

I totally agree... Let me see if I can add/modify one.

@john-science
Copy link
Member

@albeanth Any progress? Or just thoughts on testing?

@albeanth
Copy link
Member Author

albeanth commented Jan 24, 2023 •
edited by john-science
Loading

Getting a decent test in is slightly tricky since _performTightCoupling calls on interactAllCoupled and tries to actually do stuff. So I'm working on getting an operator put together that avoids doing stuff but that can still write out a DB.

@john-science
Copy link
Member

You could just subclass OperatorSnapshots, but make interactAllCoupled() do nothing (pass). Or it could do very little.

You could also use Mock.

@john-science john-science merged commit 4bed3e8 into terrapower:main Jan 24, 2023
drewj-usnctech added a commit to drewj-usnctech/armi that referenced this pull request Jan 25, 2023
@drewj-usnctech
Merge remote-tracking branch 'terrapower/main' into drewj-usnctech/10…
83cbd1f 
…30/assembly-1d-parent-class

* terrapower/main:
  Updating B4C docstring for assumed TD frac (terrapower#1117)
  bug fix for xsGroup manager w/ coupling (terrapower#1118)
  Add neutronics category to linPow parameter (terrapower#1119)
  Refactoring Material to not subclass Composite (terrapower#1062)
  Exposing tight coupling to `OperatorSnapshots` (terrapower#1113)
@albeanth albeanth deleted the tightCouplingForSnapshots branch February 15, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@opotowsky opotowsky opotowsky approved these changes

@keckler keckler Awaiting requested review from keckler

Assignees
No one assigned
Labels
bug Something is wrong: Highest Priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@albeanth @john-science @opotowsky