search

arfc / predicting-the-past

The predicting the past project.
BSD 3-Clause "New" or "Revised" License
1 stars 3 forks source link

Coordinate merge(pris, webscrape) #45

Closed gyutaepark closed 6 years ago

gyutaepark commented 6 years ago

This PR should be reviewed after #42. merge_coordinates.py was written and run to merge coordinate information from webscrape (https://github.com/arfc/webscraping), and PRIS (reactors_pris_2016.csv).

Original pris csv file have been renamed to reactors_pris_2016.original.csv Modified pris csv file used for merging is renamed to reactors_pris_2016.modified.csv Resulting csv file with PRIS data and coordinates is named as reactors_pris_2016.csv.

This PR resolves #40 once merged.

katyhuff commented 6 years ago

@jbae11 , can you review this ?

jbae11 commented 6 years ago

@gyutaepark Awesome work! However, it seems like the previous PR #42 has not been merged yet. Also, looking at the large number of files changed, @katyhuff 's comment of not having the generated files in the repository are not reflected. Please do ping me again once these two things are done! Thanks!

gyutaepark commented 6 years ago

@jbae11 Hi, tnow that #42 is merged and closed, could you review this PR please?

katyhuff commented 6 years ago

This still seems to have some conflicts (see import_data/reactors_pris_2016.csv)

gyutaepark commented 6 years ago

The conflict has been resolved. Re-requesting a review to @jbae11 !

jbae11 commented 6 years ago

@gyutaepark nice job! Is there a need to have three pris.csv files? I maybe understand the need to have the original, but the modified one seems redundant, since it shows nothing much new. Can't the coordinate values be simply appended to the original csv? Thanks!

gyutaepark commented 6 years ago

@jbae11 I agree. The script uses reactors_pris_2016.modified.csv at this moment to produce reactors_pris_2016.csv. This was a bad workflow decision that I've made in the past. I will create an issue and a PR that should be merged into this PR to fix this.

I agree that we should be using the reactors_pris_2016.original.csv to produce the output.

pep8speaks commented 6 years ago

Hello @gyutaepark! Thanks for updating the PR.

Line 515:9: E722 do not use bare except'

gyutaepark commented 6 years ago

@jbae11 The aforementioned comment has been now implemented in this PR. Is this now ready for merge? Or do you need to review the PR further?

Edit: The use of Bare Except is currently not to be fixed for this PR. We will need to discuss about this in a future issue.

jbae11 commented 6 years ago

@gyutaepark Thank you for your work. I think it's ready to merge now!

gyutaepark commented 6 years ago

Approved and merged