-
Notifications
You must be signed in to change notification settings - Fork 12
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
Reactor Deployment Script #168
Conversation
Hello @nsryan2! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-08-30 17:40:58 UTC |
I'm ignoring the pep8speaks comment about the module level import because the path needs to be specified in the tests to allow for importing the deployment_script (as with the other tests in this repo). This issue is documented in #148. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! My main overall comment is that I think a class structure would work well for this and let you remove a good chunk of repeated doc strings / code.
scripts/reactor_deployment.py
Outdated
for reactor in ar_dict.keys(): | ||
if f'num_{reactor}' not in df: | ||
df[f'num_{reactor}'] = 0 | ||
else: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make this into a class method, this could be a function call rather than repeated
Thanks for the suggestions @LukeSeifert, I see what you mean about a class. I'll reconfigure to incorporate that change. I'll say, it's not how the rest of the scripts are setup and that's probably to their detriment. This feedback would be good to connect with #58 |
I see what you mean, maybe it would be worth putting that off until a full refactor. However you decide, let me know once you're ready for me to take another look. |
@LukeSeifert thanks for the suggestions on improving the code. I have implemented all of them except for the class idea. I agree that it would improve this code, but I am hesitant to make the change now without thinking more broadly about the other scripts in this repo. I will make a note of this in the existing issue and start thinking about those changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
This PR creates a new script in the repo that calculates when and how many reactors should be deployed with 4.5 deployment schemes.
Deployment Functions
There are corresponding tests and documentation in the style of the repository.