Skip to content

GeoJson Marker#957

Merged
Conengmo merged 6 commits intopython-visualization:masterfrom
jtbaker:geojsonmarker
Dec 28, 2020
Merged

GeoJson Marker#957
Conengmo merged 6 commits intopython-visualization:masterfrom
jtbaker:geojsonmarker

Conversation

@jtbaker
Copy link
Contributor

@jtbaker jtbaker commented Sep 5, 2018

Ok, I've reworked the GeoJsonMarkers PR to have the commits staged more logically so that they are easier to review. The class is works best with is CircleMarker - it works with normal Marker/Icons too, but since they do not have a .setStyle method they can't have styles set dynamically using style_function.

Closes #1059.

@jtbaker jtbaker changed the title GeoJsonUpdateMarker GeoJson Marker Sep 19, 2018
@Conengmo
Copy link
Member

Conengmo commented Sep 28, 2018

Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

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

Interesting PR Jason! Your example notebook is helpful in seeing what you're trying to add, and I'm positive about working towards getting this merged.

I haven't really dived into your changes to the GeoJson template yet, first wanted to see make clear what the scope of this PR is. I have added some comments about whether certain things should be in this PR.

I'm wondering how necessary it is to pass a folium Marker or CircleMarker object. You're not really using the objects, except for getting the options. But in your notebook it seems your main goal is to update these options for each feature/marker. So the default options get overwritten anyway. How would it be if you let users pass the wanted marker type as a string?

How is point geometry currently handled by the way?

@jtbaker
Copy link
Contributor Author

jtbaker commented Sep 28, 2018

Thanks for the review and feedback, @Conengmo! I'll work through your comments and address them inline.

@jtbaker
Copy link
Contributor Author

jtbaker commented Sep 28, 2018

I'm wondering how necessary it is to pass a folium Marker or CircleMarker object. You're not really using the objects, except for getting the options. But in your notebook it seems your main goal is to update these options for each feature/marker. So the default options get overwritten anyway. How would it be if you let users pass the wanted marker type as a string?

I think my main goal was to follow folium's current convention with existing class structures how people are already familiar with using them. They can optionally pass the same styled marker for every feature point, style the marker based on the GeoJson properties, or a mixture of both: i.e. a base radius and weight, and then choose fill_color or additional properties in the style_function logic.

What do you think about this approach?

How is point geometry currently handled by the way?

Without a pointToLayer function in the callback, leaflet renders points with the standard Font-Awesome blue marker like the one in these examples: https://leafletjs.com/examples/quick-start/

@ocefpaf
Copy link
Member

ocefpaf commented Oct 11, 2018

@jtbaker this LGTM but I'll leave for @Conengmo to take a second look and merge b/c he is more familiar with the code base lately.

Thanks for the PR BTW! It is a nice improvement!

@jtbaker
Copy link
Contributor Author

jtbaker commented Oct 11, 2018

Thanks @ocefpaf! For the life of me, I can't figure out how to get the .xml files out of version control - I've tried reverting the commits, rebasing my master, etc, but they're still showing up. I'm not sure what to do - maybe I need to close my fork and start a new one.

@Conengmo
Copy link
Member

Conengmo commented Oct 12, 2018

I think my main goal was to follow folium's current convention with existing class structures how people are already familiar with using them. They can optionally pass the same styled marker for every feature point, style the marker based on the GeoJson properties, or a mixture of both: i.e. a base radius and weight, and then choose fill_color or additional properties in the style_function logic.

Sounds good. I hope your example notebook will learn power users how to do more complicated stuff.

I can't figure out how to get the .xml files out of version control

They seem already gone? They're not in the files changed list.

Thanks for replying on my comments. Some things have to be resolved:

  • making sure the _name attributes are consistent throughout folium
  • splitting of things that should be a separate PR
    • (I hope I'm not boring you with this, but I think for more mature libraries it's important other users can easily find if something changed why that was done.)
  • adding a check on location is not None in a new Marker.render() method

Your changes to the GeoJson template look good to me, but I'll have a better look this weekend, I have to go now.

Thanks for your patience @jtbaker!

@Conengmo Conengmo added the waiting for changes This PR has been reviewed and changes are needed before merging label Oct 15, 2018
@ocefpaf ocefpaf force-pushed the master branch 3 times, most recently from 53546b8 to 9f2299a Compare February 26, 2019 19:49
@FabeG
Copy link
Contributor

FabeG commented May 10, 2019

Hi @jtbaker, this is a very interesting and useful PR ! Do you plan an update in order to merge it ?

@pshassett
Copy link

Hi all. Really appreciate this project and would love to use these features...

@jtbaker I can see you put a lot of work into this. How can we get it to pass test and get merged?

@f2daz
Copy link

f2daz commented Jun 4, 2020

Hi all. Really appreciate this project and would love to use these features...

@jtbaker I can see you put a lot of work into this. How can we get it to pass test and get merged?

Can I help testing and if yes, how? ;)

Add handling for other icons

Overload render method to add runtime validation

On location field

Use name attribute in error reporting
@jtbaker
Copy link
Contributor Author

jtbaker commented Dec 14, 2020

I got inspired by the interest in this PR and some of the issues it addresses #1059, #1345, #1401 and rebased/refactored against the current state of the repo. The decoupling of some of the pieces of the GeoJSON logic since I last worked on it made it much easier. I believe most of the issues that were raised when this was last discussed have been addressed, but I'd be happy to hear any additional feedback seen it has been quite some time.

I've also added a few tests and an example notebook with a few examples of usage. All of the tests are now passing the CI test pipeline as well. Hopefully if this is able to make it in, it's useful to someone.

@Conengmo Conengmo removed the waiting for changes This PR has been reviewed and changes are needed before merging label Dec 20, 2020
@Conengmo Conengmo added the waiting for review PR is waiting to be reviewed label Dec 20, 2020
@Conengmo Conengmo added ready PR is ready for merging and removed waiting for review PR is waiting to be reviewed labels Dec 28, 2020
Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

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

Nice work Jason!

I made two changes: small code style changes and added an example with Marker to the notebook and ran the notebook. Update: also added a docstring.

I'll merge this when the tests passed.

@Conengmo Conengmo merged commit a7a6ab6 into python-visualization:master Dec 28, 2020
@jtbaker
Copy link
Contributor Author

jtbaker commented Dec 28, 2020

Nice work Jason!

Thanks for taking the time to review and your updates Frank! Glad this was finally able to get in.

I made two changes: small code style changes and added an example with Marker to the notebook and ran the notebook. Update: also added a docstring.

Perfect. I was having some issues having the notebook's "run" cells passing the linter in CI, so left them unexecuted in VC.

@jtbaker jtbaker deleted the geojsonmarker branch December 28, 2020 15:17
@lhoupert
Copy link

Hi guys, it seems that the test notebook is not available https://nbviewer.jupyter.org/github/python-visualization/folium/blob/master/examples/GeoJSONMarker.ipynb

@l-huff
Copy link

l-huff commented Jul 26, 2021

Hello!
I would also like to view the test notebook...

@jtbaker
Copy link
Contributor Author

jtbaker commented Aug 5, 2021

@lhoupert @l-huff that link works for me. Do any of the other notebooks in the examples directory work for you?

@goanes
Copy link

goanes commented Nov 15, 2021

Hi I see the example for the marker which work great.
in my GeoJson data, I have two type of point.
I would like to customyse the icon depending on the type of the point/marker.

I first try to customyze the icon, but it did not work for me.

icon = folium.features.CustomIcon(icon_image="Flamme.PNG", icon_size =(15,15))
marker = folium.Marker(icon=icon)
folium.GeoJson(gdf, name="point", style_function=style_function, marker=marker ).add_to(m)

I have also try to put that in the style but I did not manage to do that
def style_function(feature):
if 'Point' == feature.get('geometry').get('type'):
icon = folium.Icon(icon="star")
return {"markerColor" : "#FF0000", "markerIcon" : icon}

Any idea ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready PR is ready for merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

control markers using GeoJson

10 participants