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

Add Timeline plugin #1870

Merged

Conversation

hansthen
Copy link
Collaborator

@hansthen hansthen commented Feb 4, 2024

I closed the earlier pull request for the timeline functionality as I was not happy with the approach. I think a separate plugin is better. I think the difference in functionality is sufficient to warrant its own plugin.

Besides the ability to use end time, this plugin also does not require a array of times array with matching elements in a geometry array. Just giving each feature a start and end is simpler.

Another timeline specific functionality, is that it allows multiple layers to be controlled by a single TimeLineSlider control.

Lastly, the Timeline class inherits from GeoJson, so it can use GeoJsonPopup and GeoJsonTooltip objects.

@hansthen
Copy link
Collaborator Author

hansthen commented Feb 5, 2024

pre-commit.ci autofix

@Conengmo Conengmo self-requested a review February 19, 2024 10:56
@hansthen
Copy link
Collaborator Author

hansthen commented Mar 5, 2024

Btw, if you think this plugin is best served in its own repository, I am fine with that as well.

@Conengmo
Copy link
Member

Conengmo commented Mar 5, 2024

Sorry for the delay in reviewing, I will get to it shortly. I did the easy things first :)

@hansthen
Copy link
Collaborator Author

hansthen commented Mar 5, 2024

pre-commit.ci autofix

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.

I'd like to have this plugin in our repo, since it seems like a better approach than our current timestamped geojson plugin. But some work is needed to get it mergeable.

I added some comments based on the code alone, I didn't test it yet because the example is missing data.

We'll also need:

  • an entry in our documentation
  • list this plugin in the 'plugins' page in the docs. We'll have to make sure that the description of this one and of timestamped geojson show the difference between the two.
  • importing both classes in folium/plugins/init.py

requirements.txt Outdated Show resolved Hide resolved
folium/plugins/timeline.py Outdated Show resolved Hide resolved
folium/plugins/timeline.py Outdated Show resolved Hide resolved
folium/plugins/timeline.py Outdated Show resolved Hide resolved
folium/plugins/timeline.py Outdated Show resolved Hide resolved
folium/plugins/timeline.py Show resolved Hide resolved
folium/plugins/timeline.py Outdated Show resolved Hide resolved
folium/plugins/timeline.py Outdated Show resolved Hide resolved
hansthen added 5 commits March 7, 2024 07:43
Updated the documentation for timestamped_geojson to
clarify the difference with timeline.

Also used a simpler code example in timeline.py docstring.
Copy link
Collaborator Author

@hansthen hansthen Mar 7, 2024

Choose a reason for hiding this comment

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

The reference to the Skeate repo in line 42 is temporary, to make the tests pass. Once the PR is accepted I will change this to point to data inside our own repository.

@hansthen
Copy link
Collaborator Author

hansthen commented Mar 7, 2024

I added documentation and tests. I also updated the documentation of timestamped_geojson to make the reader aware of the two plugins. The tests (for now) refer to an example data file (borders.json) in the skeate repo. This is just to make the tests pass until this PR is accepted. I will update it to point to the Folium repo once the PR is accepted.

@hansthen hansthen requested a review from Conengmo March 7, 2024 19:45
@hansthen
Copy link
Collaborator Author

Any chance to have a look at the updated PR?

@hansthen
Copy link
Collaborator Author

@Conengmo @ocefpaf I know Folium is a volunteer project and I greatly appreciate your work on it. It is hard to make time free next to work and family obligations. Thank you both for the effort you put in the project!

I feel a bit guilty for adding to your workload by spamming you with Pull Requests. Is there any way I can help that will lessen the demands placed on you?

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.

Thanks for addressing my comments! Documentation looks good 👍 I think we're really close here, some minor comments and proposed steps for hosting the example data ourselves first. After that we should be able to merge this one!

docs/user_guide/plugins/timeline.md Outdated Show resolved Hide resolved
docs/user_guide/plugins/timestamped_geojson.md Outdated Show resolved Hide resolved
folium/plugins/timeline.py Outdated Show resolved Hide resolved
folium/plugins/timeline.py Show resolved Hide resolved
folium/plugins/timeline.py Outdated Show resolved Hide resolved
folium/plugins/timeline.py Outdated Show resolved Hide resolved
@Conengmo Conengmo changed the title Leaflet timeline controller Add Timeline plugin Apr 3, 2024
@Conengmo
Copy link
Member

Conengmo commented Apr 3, 2024

I know Folium is a volunteer project and I greatly appreciate your work on it. It is hard to make time free next to work and family obligations. Thank you both for the effort you put in the project!

Thanks for your kind words, I really appreciate it 🙏

I feel a bit guilty for adding to your workload by spamming you with Pull Requests. Is there any way I can help that will lessen the demands placed on you?

Appreciate the question, but I'm happy to have high quality contributions, that's not spam! I have two thoughts. First to make PRs that are easy to review and merge. Clear description, complete, and most importantly simple and concise, so no unnecessary stuff and simple code. Second to help keep Folium maintainable in the future by being critical of the tradeoff between added complexity and new features.

If you're interested, you can also become more involved with maintaining Folium. Helping triage issues would be very useful. And also help review PRs other than your own. Are you interested in something like that?

@hansthen
Copy link
Collaborator Author

hansthen commented Apr 3, 2024

If you're interested, you can also become more involved with maintaining Folium. Helping triage issues would be very useful. And also help review PRs other than your own. Are you interested in something like that?

I can help with both. I did start with the issue list to see if there were things I could work on or reproduce. For code reviews, how would you prefer to start? Will you use the github "request code review" feature to assign PRs to me?

@Conengmo
Copy link
Member

Conengmo commented Apr 3, 2024

I can help with both.

That's great! I added you as a collaborator on Folium. You can now also close issues or apply labels. And you can merge PRs as well. Please use this responsibly: not merging your own code without review, and for now to start get a second opinion before merging anything consequential.

For code reviews, how would you prefer to start? Will you use the github "request code review" feature to assign PRs to me?

Yes, but you can also take initiative to review any issue or PR that has your interest and that you feel you could help with.

@Conengmo
Copy link
Member

Conengmo commented Apr 3, 2024

@ocefpaf @martinfleis wanted to let you know as well I invited @hansthen to become a collaborator, see https://github.com/python-visualization/folium/settings/access.

@hansthen hansthen requested a review from Conengmo April 4, 2024 11:47
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.

Looks like examples/data/borders.json is still in this PR.

Nearly there!

folium/plugins/timeline.py Outdated Show resolved Hide resolved
@hansthen
Copy link
Collaborator Author

hansthen commented Apr 4, 2024

Looks like examples/data/borders.json is still in this PR.

Nearly there!

I used the smaller copy from the examples (also updated in the tests).

@hansthen hansthen requested a review from Conengmo April 4, 2024 13:00
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.

Great, thanks!

@Conengmo Conengmo merged commit 007fa8c into python-visualization:main Apr 4, 2024
10 checks passed
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.

2 participants