Skip to content
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

mclp and lscp jupyter notebooks using osrm routing #221

Merged
merged 6 commits into from
Mar 15, 2022

Conversation

TimMcCauley
Copy link
Contributor

Greetings @martinfleis @jGaboardi - I refactored the notebook a little and added some comments. Please let me know what you think. Happy to work in feedback if you have ideas or comments. I'd go for LSCP up next (please choose your city :) ) !

@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #221 (3f895f1) into main (72fb50b) will increase coverage by 0.2%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #221     +/-   ##
=======================================
+ Coverage   65.2%   65.5%   +0.2%     
=======================================
  Files         23      23             
  Lines       2466    2466             
  Branches     535     535             
=======================================
+ Hits        1609    1614      +5     
+ Misses       783     774      -9     
- Partials      74      78      +4     
Impacted Files Coverage Δ
spopt/_version.py 33.8% <0.0%> (+1.5%) ⬆️

@jGaboardi
Copy link
Member

Thanks so much for this @TimMcCauley! Here are some comments:

  • Since folium and routingpy are needed here, but not for spopt in general, we may want to have a statement about that in the notebook description.
  • It looks like the notebook starts from cell 209. Can you rerun so it starts from cell 1?
  • There are some long lines of code in there. We should be trying to stick to PEP8 guidelines as much as possible, but we go to a line length of 90 frequently. You can reformat manually or use black within the notebook with drillan/jupyter-black.

@TimMcCauley
Copy link
Contributor Author

Thank you @jGaboardi - I took care of your points and pushed an update.

Copy link
Member

@gegen07 gegen07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimMcCauley I didn't know about the routingpy package. That's a great one, nice addition.

I have some points:

  • I think the map is kinda small, mainly when we got the results. We could improve increasing the zoom_start to 15 and change the start location to 37.748273, -122.410558.
  • The mission district geojson structure is hardcoded. We could change it by using Nominatim API, it's just a request. I'm also fine if @jGaboardi and @martinfleis think that hardcoded polygon is ok in that case.
  • The polygon that you are using is a merge of two districts I think, Bernal Heights and Mission District. It would be good to remind that in the description or even in the comments that you did above the polygon definition line.

Awesome notebook!!! 🎉

@TimMcCauley
Copy link
Contributor Author

@gegen07 thank you - I updated it & it looks better now. I'd keep the nominatim request out for this NB if it's ok but I am currently writing one for Toronto and LSCP and I will make sure to include it there.

@TimMcCauley
Copy link
Contributor Author

I added the LSCP notebook in this PR, too. You can find it in notebooks/lscp_gis.ipynb or check it out here - let me know what you think. I made use of the Overpass API this time @gegen07 - let me know what you think (the use case is a little exotic but why not! ;-) )

@jGaboardi
Copy link
Member

Very nice with the LSCP notebook. One small typo I saw between cells 2 and 3 where Solving the MCLP should be Solving the LSCP.

@TimMcCauley
Copy link
Contributor Author

Very nice with the LSCP notebook. One small typo I saw between cells 2 and 3 where Solving the MCLP should be Solving the LSCP.

Oh, right. Fixed that now!

Copy link
Member

@gegen07 gegen07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loved this LSCP notebook. LGTM!

@gegen07 gegen07 changed the title mclp jupyter notebook using osrm routing mclp and lscp jupyter notebooks using osrm routing Mar 15, 2022
@jGaboardi
Copy link
Member

Think this is good for merging. Thanks for the contribution @TimMcCauley 🎉 !

@jGaboardi jGaboardi merged commit d7b1f75 into pysal:main Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants