Skip to content

Update on ImageOverlay, and example nb#293

Merged
ocefpaf merged 3 commits intopython-visualization:masterfrom
BibMartin:image_overlay
Dec 15, 2015
Merged

Update on ImageOverlay, and example nb#293
ocefpaf merged 3 commits intopython-visualization:masterfrom
BibMartin:image_overlay

Conversation

@BibMartin
Copy link
Copy Markdown
Contributor

I hope this addresses #291.

What's new:

At the cost of doing this, I wonder whether I shall not move ImageOverlay to plugins, as discussed in #280.

@ocefpaf
Copy link
Copy Markdown
Member

ocefpaf commented Dec 7, 2015

At the cost of doing this, I wonder whether I shall not move ImageOverlay to plugins, as discussed in #280.

👍

@BibMartin
Copy link
Copy Markdown
Contributor Author

@ocefpaf
It would require to drop map.image_overlay method, and force the users to do

map.add_children(ImageOverlay(...))

Otherwise, the main folium script will depend on plugins submodule.
Two questions:

  • What shall be done for v0.2.0 ? Drop, deprecation,nothing ?
  • What's the target for later ?

I'd vote for:

  • embedding the import folium.plugins.ImageOverlay inside the method Map.image_overlay
  • add a deprecation warning in the method.
  • drom this methods in v0.2.1.

@andrewgiessel your opinion would be welcome ; ImageOverlay is your baby 😉

@ocefpaf
Copy link
Copy Markdown
Member

ocefpaf commented Dec 7, 2015

I am OK with your strategy.

@BibMartin
Copy link
Copy Markdown
Contributor Author

Done.

We still have two functions in utilities.py that require numpy :

  • split_six : that needs a rewrite anyway.
  • write_png : that cannot be moved to plugins, because it's used by several Elements. But it's possible to rewrite it without numpy.

@andrewgiessel
Copy link
Copy Markdown
Contributor

This looks great! Thanks guys! In brief, what was the error with the
mercator projection? you have my :+1

On Mon, Dec 7, 2015 at 9:20 AM Martin Journois notifications@github.com
wrote:

Done.

We still have two functions in utilities.py that require numpy :

  • split_six : that needs a rewrite anyway.
  • write_png : that cannot be moved to plugins, because it's used by
    several Elements. But it's possible to rewrite it without numpy.


Reply to this email directly or view it on GitHub
#293 (comment)
.

@BibMartin
Copy link
Copy Markdown
Contributor Author

Squashed.

@andrewgiessel I guess the problem was (and still is) that you cannot draw an image from lat=-90 to lat=+90 ; because the mercator projection square has limits at -85.0513 to 85.0513.

If you replace

min_lat=lat.min(), max_lat=lat.max()

by

min_lat=-85.0513, max_lat=85.0513

then you get the expected graph.

@ocefpaf
Copy link
Copy Markdown
Member

ocefpaf commented Dec 9, 2015

I am 👍 to the plugin approach.

I am on the fence about the mercator transform here. My problem with this solution is that, in the previous transform and in the cartopy warp_array, we could leave the limits alone and still get the correct result [1]. By forcing the user to change to min_lat=-85.0513, max_lat=85.0513 when the limits are -90, 90 we add an extra step. I think we should cover that be covered in the transformation operation in some way. Maybe clipping the lat to 85.0513 when it goes over?

[1] http://nbviewer.ipython.org/gist/ocefpaf/954e65fa83e116eedd3e

@BibMartin
Copy link
Copy Markdown
Contributor Author

@ocefpaf
👍
I guess 72586fe does the job, right ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks correct to me in this example: http://nbviewer.ipython.org/gist/ocefpaf/4e30b6b3d2426e6294d1

I do note that warping the image, both cartopy and the old function we had, looks "different" (not as smooth as the current version). Not sure why and I haven't investigate what goes under the cover.

I will try to "beef up" the tests to check this and avoid future regressions.

@ocefpaf
Copy link
Copy Markdown
Member

ocefpaf commented Dec 14, 2015

@BibMartin I tuned out for a while. (Sorry... Day job calling.)

Is this PR OK for another round of review?

@BibMartin
Copy link
Copy Markdown
Contributor Author

@ocefpaf

Is this PR OK for another round of review?

Yes, I guess (hope) it's okay.

ocefpaf added a commit that referenced this pull request Dec 15, 2015
Update on ImageOverlay, and example nb
@ocefpaf ocefpaf merged commit 697ef75 into python-visualization:master Dec 15, 2015
@ocefpaf
Copy link
Copy Markdown
Member

ocefpaf commented Dec 15, 2015

Thanks @BibMartin!

Folium v0.2.0 is almost ready...

@ocefpaf ocefpaf added bug An issue describing unexpected or malicious behaviour documentation Documentation about a certain topic should be added labels Feb 12, 2016
@ocefpaf ocefpaf added this to the v0.2.0 milestone Feb 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug An issue describing unexpected or malicious behaviour documentation Documentation about a certain topic should be added

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants