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

Edit tight coupling runLogs + refactor tight coupling unit tests #1172

Merged
merged 16 commits into from
Feb 16, 2023
Merged

Edit tight coupling runLogs + refactor tight coupling unit tests #1172

merged 16 commits into from
Feb 16, 2023

Conversation

albeanth
Copy link
Member

@albeanth albeanth commented Feb 9, 2023 •
edited
Loading

Description

This PR addresses the following:

  1. Closes Tight Coupling: Convergence Summary should not specify norm type #1115 and Tight Coupling: add runLog stating physics have converged #1162
  2. Corrects a minor typo from https://github.com/terrapower/armi/pull/1173/files#r1103086438.
  3. Refactors the growing number of tight coupling unit tests.
    • Moves them to their own testing class that does not rely on loadTestReactor; these tests don't need a full reactor so there's no reason to build one.

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.
    • Probably not needed here. No new features or user-impacting bug fixes.
  • 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 cleanup Code/comment cleanup: Low Priority label Feb 9, 2023
@albeanth albeanth changed the title Cleanup tc run logs Cleanup tight coupling run logs Feb 9, 2023
@albeanth albeanth mentioned this pull request Feb 10, 2023
Merged
7 tasks
onufer
onufer previously requested changes Feb 10, 2023
View reviewed changes
armi/operators/operator.py Outdated Show resolved Hide resolved
@albeanth
make skipCycles a tuple
f9177ec 
- resolves a wierd discrepancy where a unit test would fail in the debugger if left as a generator
@albeanth
split up _performTightCoupling unit tests
03d0ec1 
- helps with readability and isolation of different assertions
@albeanth
refactor tight coupling tests
f2ebfe1 
- move to their own class and remove need for loadTestReactor
@albeanth
change runLog.extra -> runLog.important
7b7cbf8
@albeanth albeanth marked this pull request as ready for review February 10, 2023 21:08
@albeanth albeanth requested a review from onufer February 10, 2023 21:08
albeanth
albeanth commented Feb 10, 2023
View reviewed changes
armi/operators/tests/test_operators.py Outdated
class TestTightCoupling(unittest.TestCase):
def setUp(self):
self.cs = settings.Settings()
settings.setMasterCs(self.cs)
Copy link
Member Author

Choose a reason for hiding this comment

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

@john-science this is the culprit.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, if this HAS to go in for this PR, I will hold my nose and do it.

But we WILL have to add it to the list to remove ASAP:

#930

@albeanth albeanth changed the title Cleanup tight coupling run logs Edit tight coupling runLogs + refactor tight coupling unit tests Feb 10, 2023
@albeanth
a few more CONF_XYZ swaps
f2cfe8b 
- just for tight coupling settings for now to keep scope appropriate
@albeanth
put back cs object...
3eb418f
opotowsky
opotowsky reviewed Feb 14, 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.

Some comments and suggestions

armi/operators/tests/test_operators.py Outdated Show resolved Hide resolved
armi/operators/tests/test_operators.py Show resolved Hide resolved
armi/operators/tests/test_operators.py Show resolved Hide resolved
albeanth and others added 4 commits February 14, 2023 15:11
@albeanth @opotowsky
sneak in test for Arrielle
454c245 
Co-authored-by: Arrielle Opotowsky <arrielle.opotowsky@gmail.com>
@albeanth @opotowsky
add assertion that db runLog is NOT written
7c48240 
Co-authored-by: Arrielle Opotowsky <arrielle.opotowsky@gmail.com>
@albeanth
black formatting
4835637
@albeanth
fix unit test
54041a5
opotowsky
opotowsky approved these changes Feb 15, 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.

Thanks for adding a little extra pizazz to a unit test for me!

@opotowsky
Copy link
Member

opotowsky commented Feb 15, 2023 •
edited
Loading

To unblock this PR from being merged, I'm going to dismiss Mark's review since the changes he requested are addressed.

@opotowsky opotowsky dismissed onufer’s stale review February 15, 2023 00:14

Changes have been addressed

opotowsky
opotowsky reviewed Feb 15, 2023
View reviewed changes
armi/operators/tests/test_operators.py
self.assertTrue(self.o.couplingIsActive())
self.o.cs[CONF_TIGHT_COUPLING] = False
Copy link
Member

Choose a reason for hiding this comment

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

above you have self.cs defined than an operator init-ed from that, so can this be self.cs[CONF_TIGHT_COUPLING] = False?

Copy link
Member

Choose a reason for hiding this comment

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

same applies below in lines 146, 152, 224

I guess it doesn't actually matter

Copy link
Member Author

Choose a reason for hiding this comment

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

It could since both self.cs and self.o.cs point to the same cs object in memory. My thoughts on the self.o.cs route: once we get rid of set/getMasterCs, self.cs and self.o.cs could be different. So the assertions should live on self.o.cs I think. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

fair!

This PR is....gasp...READY

@opotowsky opotowsky merged commit 7a91be9 into terrapower:main Feb 16, 2023
@albeanth albeanth deleted the cleanup_TC_runLogs branch February 16, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@opotowsky opotowsky opotowsky approved these changes

@john-science john-science Awaiting requested review from john-science

@onufer onufer Awaiting requested review from onufer

Assignees
No one assigned
Labels
cleanup Code/comment cleanup: Low Priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tight Coupling: Convergence Summary should not specify norm type
4 participants
@albeanth @opotowsky @john-science @onufer