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

Cleanup Interface/Event runLog.header #1178

Merged
merged 5 commits into from
Feb 14, 2023

Conversation

albeanth
Copy link
Member

@albeanth albeanth commented Feb 10, 2023

Description

Prior to this PR, the Operator would print out the wrong information for tightly coupled calculations. Rather then print the current iteration number, the operator would print "cycle" before printing the iteration number.

E.g., within cycle 0 time node 0, the following would get printed to the stdout.

===========  Triggering EveryNode - cycle 0, node 0 Event ===========
=========== 01 - main                           EveryNode - cycle 0, node 0 ===========
=========== 02 - fissionProducts                EveryNode - cycle 0, node 0 ===========
=========== 03 - xsGroups                       EveryNode - cycle 0, node 0 ===========
...
===========  Triggering Coupled - cycle 0 Event ===========
=========== 01 - main                           Coupled - cycle 0 ===========
=========== 02 - fissionProducts                Coupled - cycle 0 ===========
=========== 03 - xsGroups                       Coupled - cycle 0 ===========
... 
===========  Triggering Coupled - cycle 1 Event ===========
=========== 01 - main                           Coupled - cycle 1 ===========
=========== 02 - fissionProducts                Coupled - cycle 1 ===========
=========== 03 - xsGroups                       Coupled - cycle 1 ===========

This should rather read the following.

===========  Triggering EveryNode - cycle 0, node 0 Event ===========
=========== 01 - main                           EveryNode - cycle 0, node 0 ===========
...
===========  Triggering Coupled - iteration 0 Event ===========
=========== 01 - main                           Coupled - iteration 0 ===========
...
===========  Triggering Coupled - iteration 1 Event ===========
=========== 01 - main                           Coupled - iteration 1 ===========

Unit tests were also added to ensure that the correct string within Operator::expandCycleAndTimeNodeArgs gets printed for each event type and interface.


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 cleanup Code/comment cleanup: Low Priority label Feb 10, 2023
@albeanth albeanth requested review from john-science, mgjarrett, keckler and opotowsky and removed request for mgjarrett February 10, 2023 23:31
@albeanth
Copy link
Member Author

@jakehader @onufer Just an FYI.

@keckler
Copy link
Member

keckler commented Feb 10, 2023

I am a big fan, and would be an even bigger fan if it were even more descriptive, like:

===========  Triggering EveryNode - cycle 0, node 0 Event ===========
=========== 01 - main                           EveryNode - cycle 0, node 0 ===========
...
===========  Triggering Coupled - iteration 0 Event ===========
=========== 01 - main                           Coupled - cycle 0, node 0, iteration 0 ===========
...
===========  Triggering Coupled - iteration 1 Event ===========
=========== 01 - main                           Coupled - cycle 0, node 0, iteration 1 ===========

I really hate having to scroll around to know where in the calculation the printout is referring.

@albeanth
Copy link
Member Author

@keckler great idea! I originally had something like this in a git stash and had in my head that it would require a bigger API change. Turns out it didn't. And ended up resulting in a way less intrusive change! So double score.
d0d93e9

Running one of our in-house tests now to make sure the stdout is right.

@albeanth
Copy link
Member Author

albeanth commented Feb 11, 2023

@keckler great idea! I originally had something like this in a git stash and had in my head that it would require a bigger API change. Turns out it didn't. And ended up resulting in a way less intrusive change! So double score. d0d93e9

Running one of our in-house tests now to make sure the stdout is right.

Jk. The in-house test just failed because the bigger API change is in fact needed. I would suggest that we revert back to fcfbcf9 then address your suggestions post our big internal deliverable. This will require a change to the interactCoupled method on all of the interfaces. Less than ideal right now I think....

Thoughts?

@keckler
Copy link
Member

keckler commented Feb 11, 2023

No big deal, happy for this small change anyway.

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.

💯 Great description, tests are awesome, winning PR! 🏆

@opotowsky opotowsky merged commit 867ebec into terrapower:main Feb 14, 2023
@albeanth albeanth deleted the interfaceEvent_runLog branch February 15, 2023 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code/comment cleanup: Low Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants