-
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
Modified creation of cyclus input from PRIS data #115
Conversation
Hello @abachma2! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-01-13 00:04:46 UTC |
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.
I've never used cyclus, so my comments are from the perspective of an outsider-ish.
My two thoughts are:
- Templates should be maximally flexible. (This is really an opinion)
- Magic numbers should not exist without very good reason (and then they're usually not magic numbers).
@@ -63,14 +63,14 @@ | |||
</recipe> | |||
<recipe> | |||
<name>cool_spent_uox</name> | |||
<basis>atom</basis> | |||
<basis>mass</basis> |
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.
This seems like something that should be user specified.
E.g. allow for options when running create_input.py
like
python create_input.py -a # for atom basis
pyhton create_input.py -m # for mass basis
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.
As long as the IAEA PRIS database is on a mass basis, this is fine. @abachma2 feel free to resolve this conversation if that's the case.
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.
The recipe data doesn't come from PRIS. It comes from the vision recipes in database/vision_recipes/
. I don't know where that data came from originally.
Thanks for the comments @samgdotson. I made most of the changes to reflect the current structure of the raw data from PRIS. I can try to modify it to match column names as opposed to numbers, allowing for some flexibility if the raw data changes format. |
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.
I think this is better. Again, I'm not familiar with Cyclus so you might want someone else to approve before merging.
I changed some of the data containers to pandas DataFrames from lists. This should remove need for 'magic numbers' |
@@ -63,14 +63,14 @@ | |||
</recipe> | |||
<recipe> | |||
<name>cool_spent_uox</name> | |||
<basis>atom</basis> | |||
<basis>mass</basis> |
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.
As long as the IAEA PRIS database is on a mass basis, this is fine. @abachma2 feel free to resolve this conversation if that's the case.
Made two minor comments. When handled, I approve. |
Merged! |
I made changes to facilitate the creation of cyclus input files using data from the PRIS Year-end Report.
scripts/predicting-the-past-import.py
was modified to reflect the structure of the raw data from PRIS, when saved as a.csv
.scripts/create_input.py
was created to perform the actions to create the input file, mirroring what is ininputs/predicting-the-past/united_states.ipynb
.