Closed ianmilligan1 closed 7 years ago
ianmilligan1
commented
7 years ago The two external reviews and discussion can be found on this pull request.
As we've received two reviews, and revisions have been submitted, I will go through them. I will quickly reach out to the original externals to take a quick look as well, given the time that's passed.
ianmilligan1
commented
7 years ago @kimpham54 as I tackle that, could you also check out the preview link above and look at any formatting issues? I see that sometimes you have a 3-level heading followed by a code snippet which is rendering odd on Chrome. Perhaps a line of text just introducing might help?
kimpham54
commented
7 years ago @ianmilligan1 thanks for noticing the issue! it's fixed now.
ianmilligan1
commented
7 years ago Thanks! Just as an update, I've reached out to the reviewers. I think they're currently on the road but I will keep on them. 😄
ianmilligan1
commented
7 years ago And another update, the reviewers'll take another look by the end of the month!
ianmilligan1
commented
7 years ago Apologies, the reviewers have run into some snags. I'm now told we'll have reviews in two weeks. Really sorry about this Kim.
kimpham54
commented
7 years ago @ianmilligan1 thanks for the update, no problem
shawngraham
commented
7 years ago Hi folks - I've annotated the lesson with the bits and bobs where things could be clarified, expanded, or fixed. Now, I'm on a mac, so I can't speak to whether the windows bits do what they need to do. Please see my annotations in context at this hypothesis-enabled vision of the lesson
Hypothesis activity for the query uri = https://programminghistorian.github.io/ph-submissions/lessons/mapping-with-python-leaflet
shawngraham
commented
7 years ago Hi,
Please find my annotations on the lesson at this annotated layer
This is a very clear and straightforward introduction to webmapping, and a welcome addition to the programming historian. I only ran into some minor snags - eg the missing geolocator line
so things that should be trivial to sort out. Thank you for the opportunity to learn!
ianmilligan1
commented
7 years ago Wonderful, thanks @shawngraham – @kimpham54 it might be easiest for you to use the hypothesis plugin to navigate this, let me know if you have any questions.
@jburnford has finished his review and will be posting it shortly. I'll then make a few quick comments and I think we'll be in final revision territory!
jburnford
commented
7 years ago I’ve read through the lesson and don’t have any objections to publishing it. The Leaflet section is improved and provides a good basic introduction to this tool. I really like all of the exercises that include solutions. The lesson does a lot, ranging from pip and pandas to launching a website on a server with a map. I raised a few concerns with the lack of depth as the author plowed through all of these new tools and skills in the first draft, but I think this is has been addressed and this is a good lesson for students who want to push past the initial learning Python lessons and start building new skills. In that situation, it is great to introduce students to pandas and get them thinking about how to interact with tabular data programmatically. As a result, the lesson has value beyond GIS/mapping.
On Thu, Jul 27, 2017 at 5:15 PM, Ian Milligan notifications@github.com wrote:
Wonderful, thanks @shawngraham https://github.com/shawngraham – @kimpham54 https://github.com/kimpham54 it might be easiest for you to use the hypothesis plugin to navigate this, let me know if you have any questions.
@jburnford https://github.com/jburnford has finished his review and will be posting it shortly. I'll then make a few quick comments and I think we'll be in final revision territory!
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/programminghistorian/ph-submissions/issues/85#issuecomment-318512596, or mute the thread https://github.com/notifications/unsubscribe-auth/AFOiU0crFiWb0RZp389AdWIsB7ILF5Wlks5sSRn2gaJpZM4NopWX .
-- http://jimclifford.ca http://activehistory.ca/ http://tradingconsequences.blogs.edina.ac.uk/
ianmilligan1
commented
7 years ago Thanks both. Sincerely appreciate the renewed engagement @jburnford and @shawngraham!
@kimpham54 - I've had a chance to go through both revised reviews, and there's a lot of enthusiasm. I think the next step should be to go through @shawngraham's hypothesis comments... any issues accessing those?
Any chance you could find time in the next two weeks to do so (say by 16 August 2017). Then I'll go through, do one last playthrough and suggestions, and we should be good to publish shortly thereafter.
kimpham54
commented
7 years ago @ianmilligan1 @jburnford @shawngraham Thank you everyone for your comments and suggestions. I have some time tomorrow then after the 13th to make revisions. Will keep you updated!
ianmilligan1
commented
7 years ago Merged the PR, @kimpham54 – ready for me to take a gander?
kimpham54
commented
7 years ago @ianmilligan1 Yes, please do. Thank you!
ianmilligan1
commented
7 years ago Just working through this @kimpham54 (on baby nap time). I've run into a roadblock. When running:
import geopy
import pandas
from geopy.geocoders import Nominatim, GoogleV3
# versions used: geopy 1.10.0, pandas 0.16.2, python 2.7.8
def main():
io = pandas.read_csv('census-historic-population-borough.csv', index_col=None, header=0, sep=",")
def get_latitude(x):
return x.latitude
def get_longitude(x):
return x.longitude
io['latitude'] = io['Area_Name'].apply(geolocator.geocode).apply(get_latitude)
io['longitude'] = io['Area_Name'].apply(geolocator.geocode).apply(get_longitude)
io.to_csv('geocoding-output.csv')
if __name__ == '__main__':
main()I get the error:
Traceback (most recent call last):
File "geocoder.py", line 20, in <module>
main()
File "geocoder.py", line 15, in main
io['latitude'] = io['Area_Name'].apply(geolocator.geocode).apply(get_latitude)
NameError: name 'geolocator' is not definedThoughts? I've gone through all the pip installs.
jbartonthomas
commented
7 years ago It looks like maybe instantiating the geocoder got skipped. Also, if I'm reading this correctly, this code is also geocoding every point twice. I guess points probably get cached, but I think it would still be better to only run that once, then run your applies to pull out lat and long for your pts.
Jewell
On Thursday, August 17, 2017, Ian Milligan notifications@github.com wrote:
Just working through this @kimpham54 https://github.com/kimpham54 (on baby nap time). I've run into a roadblock. When running:
import geopyimport pandasfrom geopy.geocoders import Nominatim, GoogleV3# versions used: geopy 1.10.0, pandas 0.16.2, python 2.7.8 def main(): io = pandas.read_csv('census-historic-population-borough.csv', index_col=None, header=0, sep=",")
def get_latitude(x): return x.latitude
def get_longitude(x): return x.longitude
io['latitude'] = io['Area_Name'].apply(geolocator.geocode).apply(get_latitude) io['longitude'] = io['Area_Name'].apply(geolocator.geocode).apply(get_longitude) io.to_csv('geocoding-output.csv') if name == 'main': main()
I get the error:
Traceback (most recent call last): File "geocoder.py", line 20, in
main() File "geocoder.py", line 15, in main io['latitude'] = io['Area_Name'].apply(geolocator.geocode).apply(get_latitude) NameError: name 'geolocator' is not defined Thoughts? I've gone through all the pip installs.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/programminghistorian/ph-submissions/issues/85#issuecomment-323171438, or mute the thread https://github.com/notifications/unsubscribe-auth/AMrIQeW55--rdnbFCw7civnaQHZnd_1Gks5sZJT3gaJpZM4NopWX .
ianmilligan1
commented
7 years ago Great, thanks Kim. Also, this now works (no errors) but there's no output file generated.
import geopy
import pandas
from geopy.geocoders import Nominatim, GoogleV3
# versions used: geopy 1.10.0, pandas 0.16.2, python 2.7.8
def main():
io = pandas.read_csv('census-historic-population-borough.csv', index_col=None, header=0, sep=",")
def get_latitude(x):
return x.latitude
def get_longitude(x):
return x.longitude
geolocator = Nominatim()
# geolocator = GoogleV3()
# uncomment the geolocator you want to use
io['latitude'] = io['Area_Name'].apply(geolocator.geocode).apply(get_latitude)
io['longitude'] = io['Area_Name'].apply(geolocator.geocode).apply(get_longitude)
io.to_csv('geocoding-output.csv')@ianmilligan1 strange! it worked for me. Let me look into it again
kimpham54
commented
7 years ago I made a few changes, this should be the final script that you use:
import geopy
import pandas
from geopy.geocoders import Nominatim, GoogleV3
# versions used: geopy 1.10.0, pandas 0.16.2, python 2.7.8
def main():
io = pandas.read_csv('census-historic-population-borough.csv', index_col=None, header=0, sep=",")
def get_latitude(x):
return x.latitude
def get_longitude(x):
return x.longitude
geolocator = Nominatim()
# geolocator = GoogleV3()
# uncomment the geolocator you want to use
geolocate_column = io[namecolumn].apply(geolocator.geocode)
io['latitude'] = geolocate_column.apply(get_latitude)
io['longitude'] = geolocate_column.apply(get_longitude)
io.to_csv('geocoding-output.csv')
if __name__ == '__main__':
main()It should be the code segment right before the line "Do you have the script saved and ready to go? Good. Run the script from your command line by typing:"
ianmilligan1
commented
7 years ago I get this now:
Traceback (most recent call last):
File "geocoder.py", line 25, in <module>
main()
File "geocoder.py", line 19, in main
geolocate_column = io[namecolumn].apply(geolocator.geocode)
NameError: name 'namecolumn' is not definedI see namecolumn is defined in the next script?
kimpham54
commented
7 years ago @ianmilligan1 you're right. At this point I didn't introduce sys args yet, instead of io[namecolumn], it should be io['Area_Name']. PR forthcoming
ianmilligan1
commented
7 years ago Awesome! A few other quick points up until paragraph 60, where I hit another roadblock. But after pressing on, just using the first version of the csv output, things were looking good!
So a few things:
census_country.csv. Maybe we can provide it as an asset as well?geocoder.py. Maybe we should call it something new? And can we provide a screenshot after paragraph 60 of what it should look like now?For example, I ended up with something like this:
import geopy, sys
import pandas
from geopy.geocoders import Nominatim, GoogleV3
# versions used: geopy 1.10.0, pandas 0.16.2, python 2.7.8
inputfile=str(sys.argv[1])
namecolumn=str(sys.argv[2])
def main():
io = pandas.read_csv(inputfile, index_col=None, header=0, sep=",")
def get_latitude(x):
return x.latitude
def get_longitude(x):
return x.longitude
geolocator = Nominatim()
# geolocator = GoogleV3()
# uncomment the geolocator you want to use
io['helper'] = io['Area_Name'].map(str) + " " + io['Country'].map(str)
geolocate_column = io['helper'].apply(geolocator.geocode)
io['latitude'] = geolocate_column.apply(get_latitude)
io['longitude'] = geolocate_column.apply(get_longitude)
io.to_csv('geocoding-output.csv')
if __name__ == '__main__':
main()and got:
Traceback (most recent call last):
File "geocoder.py", line 6, in <module>
inputfile=str(sys.argv[1])
IndexError: list index out of rangeLater on, since it all does seem to work in Python 3 as well, alternate code for SimpleHTTPServer is – can we provide these?
python3 -m http.serveror
python3 -m http.server 8080OK! I've now gone through the lesson and am having fun with Leaflet goodness (see picture)! We're almost ready to push this up, with just a few minor things and then we'll get it up. 🎉
Final things to do:
Otherwise, for production I need:
I think with these small tweaks we'll be ready to post this up next week!
ianmilligan1
commented
7 years ago Thanks Kim! I will try to review tomorrow, and in the meantime if you could attend to bio and image thoughts here (and if not on image, I'm happy to find a candidate).
kimpham54
commented
7 years ago Bio Kim Pham is the Digital Projects and Technologies Librarian at the University of Toronto Scarborough Library. Her heart belongs to open-source software & shared, community-driven development practices in libraries.
Image https://www.loc.gov/collections/static/national-parks-maps/images/ye000013.jpg?55835.31062 found in the slideshow gallery here: https://www.loc.gov/collections/national-parks-maps/about-this-collection/
@ianmilligan1 Thank you very, very much!
ianmilligan1
commented
7 years ago Hi Kim!
Here's a soft launch of the lesson over at Programming Historian: https://programminghistorian.org/lessons/mapping-with-python-leaflet. Can you take a close look to proof it? I had to change some figure syntax and other things, so in particular we need to make sure all the links are working (I think they are).
Let's aim to try to launch this formally early next week.
kimpham54
commented
7 years ago @ianmilligan1 I read over the lesson, checked the links and checked the formatting. Looks good to me!
ianmilligan1
commented
7 years ago OK awesome! I will do a final once over and we'll begin the publicity machine tomorrow. :)
jburnford
commented
7 years ago Congratulations Kim!
On Mon, Aug 28, 2017 at 2:08 PM, Ian Milligan notifications@github.com wrote:
OK awesome! I will do a final once over and we'll begin the publicity machine tomorrow. :)
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/programminghistorian/ph-submissions/issues/85#issuecomment-325465397, or mute the thread https://github.com/notifications/unsubscribe-auth/AFOiU7FZBrFEfG2ufNf3Tw3_50lZ_KIjks5scx5DgaJpZM4NopWX .
-- http://jimclifford.ca http://activehistory.ca/ http://tradingconsequences.blogs.edina.ac.uk/
ianmilligan1
commented
7 years ago Hazzah, and I've sent out the tweet. Please help spread the good word.
Thanks again for your wonderful lesson, @kimpham54, and to @shawngraham @jburnford for their reviews (and @jbartonthomas for their comment too!). A pleasure as always to work with you all.
🎉 🎉 🎉
kimpham54
commented
7 years ago @ianmilligan1 @shawngraham @jburnford Thank you all very much for your major contributions. I am extremely grateful for your patience. Thank you for your feedback @jbartonthomas. @ianmilligan1 A big thank you for guiding me through this process. It was a very valuable experience for me!
The Programming Historian has received the following tutorial on 'Web Mapping with Python and Leaflet' by @kimpham54. This lesson is now under review and can be read at:
http://programminghistorian.github.io/ph-submissions/lessons/mapping-with-python-leaflet
Please feel free to use the line numbers provided on the preview if that helps with anchoring your comments, although you can structure your review as you see fit.
I will act as editor for the review process. I have already read through the lesson and provided feedback, to which the author has responded.
My other role is to solicit two reviews from the community and to manage the discussions, which should be held here on this forum. You can see the reviews on this pull request, as they came in under our previous system.
I will endeavor to keep the conversation open here on Github. If anyone feels the need to discuss anything privately, you are welcome to email me. You can always turn to @amandavisconti if you feel there's a need for an ombudsperson to step in.
Anti-Harassment Policy
This is a statement of the Programming Historian's principles and sets expectations for the tone and style of all correspondence between reviewers, authors, editors, and contributors to our public forums.