US report updated with PRIS information. Reports completed and ready for review.

arfc / predicting-the-past

The predicting the past project.

BSD 3-Clause "New" or "Revised" License

1 stars 3 forks source link

US report updated with PRIS information. Reports completed and ready for review. #42

Closed gyutaepark closed 6 years ago

gyutaepark commented 6 years ago

US report is updated with PRIS reactor data. US report is proof-read and updated. Europe report is proof-read and updated.

This PR is for review purposes as detailed in #33 .

pep8speaks commented 6 years ago

Hello @gyutaepark! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. :beers:

Comment last updated on March 23, 2018 at 20:44 Hours UTC

gyutaepark commented 6 years ago

This also fixes #37 once merged.

katyhuff commented 6 years ago

Great! I'll be looking this on the plane in the next few hours. I'll upload some comments after I land. If many days pass without action, please ping me.

gyutaepark commented 6 years ago

@katyhuff Hello! This is the ping to remind you of this PR. Please feel free to review it at your convinience. Thank you.

gyutaepark commented 6 years ago

@katyhuff Hello! This is a ping just in case you've missed the previous one. Please feel free to review it at your convinience. Thank you.

katyhuff commented 6 years ago

Thanks yes, I was in the middle of a PR and then had to stop and haven't picked it back up again. It looks great! S far, I really just noticed a few grammatical issues, primarily missing words, in the first of these notebooks that I looked at. Can you please re-read UNITED_STATES.ipynb closely for missing words?

Other thoughts:

It's best to avoid caps in the filenames.
Let's make the .sqlite files available in case someone running the file wants to see the analysis without running cyclus. So, for each file 1) put it in box as a public file 2) When you run cyclus, if the cyclus executible isn't found, then pull the file from box.

gyutaepark commented 6 years ago

Thank you! I must have missed them. I'll fix these right away

gyutaepark commented 6 years ago

Sorry for the delay. I've currently went over both reports (which are very similar) to remove as many grammatical and stylistic errors as I possibly could. I'll now update other file names to be lower case and also upload a copy of the europe output file.

gyutaepark commented 6 years ago

The repository is now ready for review! I will re-request a review.

katyhuff commented 6 years ago

There are 575 files changed in this PR. Generally, it's best to avoid keeping generated files in the repository (e.g. plots). I know I said you should include the sqlite files, but the idea in general is that it's not wise to version control things that can be created with only the source files in the repository...

gyutaepark commented 6 years ago

Sorry for the delay. I'll remove the generated files and add them to gitignore!