-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Feature overlapping markers #2033
Feature overlapping markers #2033
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Nice to have this ability.
@Conengmo Do you also want to review?
_template = Template( | ||
""" | ||
{% macro script(this, kwargs) %} | ||
var {{ this.get_name() }} = (function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how you used an IFFE here. I think we should use these for all our templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I looked up this method in FastMarkerCluster, so at least one plugin already uses IFFE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this up @swtormy! I love how complete your PR is!
One thing that tripped me up a bit though is whether marker
objects should be added to the map or a feature group before being added to the OverlappingMarkerSpiderfier
object. Turns out, looking at your implementation, they should not. Because the OMS
class does create the markers in JS in its template. But in the docs example the markers are also added to the map.
That issue also led me to question whether we could simplify the template in this plugin class. Currently it contains code to create markers and popups. But we already have the Marker
and Popup
class. Ideally, we'd reuse more of those! That way, all the existing stuff around markers like tooltips, colors and icons works as well. It doesn't right now.
Suggestion, and I don't know if this is possible or reasonable: create Marker
and Popup
as usual, add them to a map or feature group as usual, and let the OMS
class only handle the spiderfying and the click events. So in the OMS
template overwrite the existing popup click events and bind them to the oms
.
In short:
- in the current implementation, should markers be added to the map? I guess no, so documentation should be clear on that limitation.
- Or, can we take the marker and popup creation out of this plugin and use the existing marker and popup classes?
- if so, should this plugin actually be a
Layer
? Or should it just be a collector of existing markers?
Apologies if that's a bit much. I'd love to hear what you think about this. And let me know if I understood it correctly! Feel free to be open about what kind of changes you feel okay with.
Thanks so much for the thoughtful review, @Conengmo! Your comments are incredibly valuable, and you've picked up on several important points that I missed - both in the code and in the documentation. Looks like my documentation decided to freelance its own narrative. Clearly, it needs a little more supervision. 😄 I completely agree that using the existing Here's an overview of the changes:
I've reworked the code to reflect this approach - please take a look at my latest implementation and let me know what you think. |
9b9de5e
to
c8b17e7
Compare
c8b17e7
to
07e536d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @swtormy, thanks so much for making these changes. I love how the code became simpler but everything, like the click events, still works.
I have basically two comments still. One about how we deal with class arguments in Folium that I should have mentioned the previous review, sorry about that. The other about a new missing use case I'd like to hear your opinion on.
I think with these out of the way we can get this merged!
Co-authored-by: Frank Anema <[email protected]>
Co-authored-by: Frank Anema <[email protected]>
I decided to try a recursive approach to finding markers, but not in the js template. In doing so, I wanted to keep the approach of minimal user interaction with the plugin so that the user doesn't have to add each token separately. I tried modifying def add_to(
self, parent: Element, name: Optional[str] = None, index: Optional[int] = None
) -> Element:
self._parent = parent
self.markers = self._get_all_markers(parent)
super().add_to(parent, name=name, index=index)
def _get_all_markers(self, element: Element) -> list:
markers = []
for child in element._children.values():
if isinstance(child, Marker):
markers.append(child)
elif hasattr(child, "_children"):
markers.extend(self._get_all_markers(child))
return markers In recursion I am looking for markers within all children, it seems that this approach should help to find markers not only in FeatureGroup but also in FeatureGroupSubGroup. I completely agree that adding more functionality increases the code and therefore the possibility of getting bugs. Therefore, if you still think that there is no need to complicate things here, I will gladly remove the recursive traversal and leave only manual addition of markers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely! This works great, good idea to do it this way. I have one tiny comment about a default value in the docstring that I'll merge myself, after that if there's nothing more from your end I'll go ahead and merge this one!
@hansthen, @Conengmo thank you for reviewing and approving the changes! 👍 I’ve tested the plugin locally, including Selenium tests, and everything passed successfully. However, if I understand correctly, there is an issue with the Selenium environment for python3.9 and 3.13 in the CI pipeline that doesn't bother with the plugin. Maybe, it might be resolved by simply restarting the failing runner? Or is the problem in my code? |
I’ve restarted the tests but they still fail… Ill take a closer look tomorrow. |
Ok, I tried to get Micromamba up on my windows, with an Ubuntu container to run selenium tests on python 3.9 and 3.13. It took longer than I thought. I'll take a closer look tomorrow as well. I want to try to rule out my tests first, something tells me that it might be because of them, even though there is no clear indication of them in the logs. |
I also have problems running the tests. It would be cool if there were some instructions on how to setup the tests to run locally as part of the developer instructions. |
@hansthen theres this list in the contributors guide: https://github.com/python-visualization/folium/blob/main/.github/CONTRIBUTING.md#contributing-code. Is there anything missing there? If I’m not mistaken Selenium downloads drivers itself these days. |
Perhaps @hansthen means that this instruction doesn't have information on how to model python 3.9 and 3.13 in ubuntu if the user is using other OS. Perhaps I'm missing something, but this instruction describes the process in which the selenium driver is loaded for the OS the user is running on. In my case it is windows and macos. But on github the test actions are performed in ubuntu. I haven't yet had time to recreate the conditions from |
I reran the same test on the main branch and it fails there as well, so it's not because of your change: https://github.com/python-visualization/folium/actions/runs/11880066659/job/33603469550 |
That makes sense, but that goes a bit to far for a contributor guide IMO to go into containerizing different OS'es :) |
I made a temporary fix for the Selenium test failure in #2034. Could one of you review it? I'll go ahead and merge this PR since the test failure is unrelated. |
Thanks for your excellent work on this @swtormy! |
Add OverlappingMarkerSpiderfier Plugin
Overview
In the pull request, I added a new OverlappingMarkerSpiderfier plugin to Folium. It solves the problem of overlapping markers on maps by “stretching” them into a distinct, scattered pattern. This feature will allow the user to work with adjacent markers, eliminating the problem described in #901.
Key Features
keepSpiderfied
,nearbyDistance
, andlegWeight
(description here).Changes in This PR
OverlappingMarkerSpiderfier
infolium/plugins/overlapping_marker_spiderfier.py
.docs/user_guide/plugins/overlapping_marker_spiderfier.md
.tests/plugins/test_overlapping_marker_spiderfier.py
.folium/plugins/__init__.py
to include the new plugin.Example Usage
Here's a simple example to demonstrate the plugin:
Addressed Issue
This PR resolves Issue #901, which requested a solution for handling overlapping markers on maps. The proposed solution integrates the OverlappingMarkerSpiderfier Leaflet plugin into Folium.
Testing
PS: Feedback is welcome! If there are additional improvements or refinements needed, I am happy to make the necessary adjustments.