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

Clarifying the inputs of plotAssemblyTypes #773

Merged
merged 1 commit into from
Jul 15, 2022
Merged

Clarifying the inputs of plotAssemblyTypes #773

merged 1 commit into from
Jul 15, 2022

Conversation

john-science
Copy link
Member

@john-science john-science commented Jul 15, 2022 •
edited
Loading

Description

The function plotAssemblyTypes() is now set up to take one of two inputs and still function. But that doesn't really jive with the Python "default" arguments functionality. So I clarify the function here.

NOTE: I also ran pylint on the two files I edited for this touched for this PR, and fixed some linting errors.

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.

@john-science john-science added the documentation Improvements or additions to documentation label Jul 15, 2022
@john-science john-science linked an issue Jul 15, 2022 that may be closed by this pull request
Optional input param in plotAssemblyTypes is not optional #701
Closed
keckler
keckler approved these changes Jul 15, 2022
View reviewed changes
Copy link
Member

@keckler keckler left a comment

Choose a reason for hiding this comment

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

Changes look good.

I would recommend that you update the PR description with the changes you made to plotBlockFlux as well, so that it is clear that these were intentional.

armi/utils/plotting.py
@@ -956,6 +958,8 @@ def __init__(
self.peak = peak
self.avgHistogram = None
self.eHistogram = None
self.peakHistogram = None
self.E = None
Copy link
Member

Choose a reason for hiding this comment

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

These changes seem random, not sure how you caught that these instance variables were required here.

Obviously nobody has used this plotter in a while...

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I just ran pylint on the two files I was editing in this PR. Based on how the code runs, these are not bugs, but it is extremely bad programming practice to define self. variables on-the-fly in the code, and not in the __init__() method.

@john-science john-science merged commit be99de9 into terrapower:main Jul 15, 2022
@john-science john-science deleted the plot_assem_cleanup branch July 15, 2022 19:54
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
@john-science @scottyak
Clarifying the inputs of plotAssemblyTypes (terrapower#773)
43b1368
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keckler keckler keckler approved these changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optional input param in plotAssemblyTypes is not optional
2 participants
@john-science @keckler