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

Polygon from list of points example #1056

Merged

Conversation

HZALK
Copy link
Contributor

@HZALK HZALK commented Jan 19, 2019

As suggested by reviewers I've created an example notebook that shows the application of convex hull and envelope to create polygons from a list of points in folium maps. Since continuous-integration did not like the sequence of my first request, this is an updated version.

@ocefpaf
Copy link
Member

ocefpaf commented Jan 21, 2019

Awesome addition to our example gallery. Thanks @HZALK!

Can you please change the last line to my_map_global instead of my_map_global.save('example_map.html') so the map is displayed in the repr?

@HZALK
Copy link
Contributor Author

HZALK commented Jan 23, 2019

Thank you, @ocefpaf :-)
I've changed the respective line.

@Conengmo
Copy link
Member

Good work @HZALK, this will make a good addition to the example gallery. I have a few comments to possibly improve it, but none of these are critical, so do what you like and let me know which ones you want to skip.

  • You use two different variable/function name formats. It's best to stick to the PEP8 one: variable_name and def function_name(). So DrawPoints() would become draw_points(). That way we have consistent styling in the folium library.
  • Don't make your lines too long. It's best if we don't have to scroll sideways to read what's going on.
  • For the example I think it would be better to show a map after each polygon type. So explain a bit about convex hull, then immediately show the code and the map with the convex hull. That way it is clear what the section is about. You can still show a final map at the end combining them.
  • The polygon line and the points have the same color, and the line runs over the points. It's hard to see the points under the line. Maybe make the points a different color/shade (and thicker) and have them put on top of the line.
  • If you use LayerControl(collapsed=False) it shows the controls, which is better I think because not everyone may know you can hover the control button in the top right corner.

@HZALK
Copy link
Contributor Author

HZALK commented Jan 26, 2019

Thank you @Conengmo for you great feedback and suggestions. I’ve addressed all points you mentioned.

@Conengmo Conengmo merged commit 45a82c6 into python-visualization:master Jan 27, 2019
@Conengmo
Copy link
Member

Merged! Thanks @HZALK for your contribution! If you want to help out again you're very welcome to do so.

@HZALK
Copy link
Contributor Author

HZALK commented Jan 27, 2019

Thanks @Conengmo. It was a pleasure. If have a new idea, I'll be glad to contribute again. :-)

@HZALK HZALK deleted the Polygon-from-List-of-Points-Example branch January 27, 2019 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants