docstring and defaults

[ocefpaf]

Addressing some of the points we raised in #187.

@andrewgiessel since this is your baby can you please take a look?

Here is a working example:
http://nbviewer.ipython.org/github/ocefpaf/folium_notebooks/blob/master/test_image_overlay_gulf_stream.ipynb

[andrewgiessel]

This looks great! Thanks for tackling it.

A comment: While I've never really been satisfied with data_projection as an argument name, I do like the explicitness of using 'mercator' and None. That said, I don't think we should incorporate arbitrary projections (like using proj4 or whatever, blech) If we use True/False, then maybe 'project_to_mercator' might be a clearer argument name. There is just a lot of ambiguity.

Also, projection only works if you use it with an array, not with a local or external file.

[ocefpaf]

I am not happy with data_projection either. To be honest I am not happy with that option
and the current implementation. The image distortion seen here worries me...

Maybe should remove that for now and try to implement that using rasterio or Cartopy.

PS: Any idea about what is going on with travis? Got it. It was the geodetic_to_mercator()

[andrewgiessel]

Ah, I think the issue is that the algorithm is for global data, not local.
So, for my global data overlays it works well. For smaller single images,
it won't. So, generally speaking, I think we 2 have different use cases
(which are both very important): user generated data (my primary use), and
landsat data (yours?). The former will tend to be global data, and this
algo will work. The later doesn't work well.

rasterio looks pretty cool. I don't know enough about cartopy to know how
to do this, but I should learn about both- do you want me to do that and
then look into a more general solution?

On Tue, Aug 18, 2015 at 9:55 AM Filipe notifications@github.com wrote:

I am not happy with data_projection either. To be honest I am not happy
with that option
and the current implementation. The image distortion seen here
http://nbviewer.ipython.org/github/ocefpaf/folium_notebooks/blob/master/test_image_overlay.ipynb
worries me...

Maybe should remove that for now and try to implement that using rasterio
https://github.com/mapbox/rasterio or Cartopy.

PS: Any idea about what is going on with travis?

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

[ocefpaf]

I am re-thinking this issue and I believe it is better to avoid any re-projection inside folium. Lets just say it takes the image in Web Mercator and leave the transformation to the user. My notebook shows one example of re-projecting Geodetic data to PlateCarree (which seems to be OK in the map.)

We can easily create example with your geodetic_to_mercator() and rasterio too. But I am leaning towards not hard-wiring this into folium.

[andrewgiessel]

Ya, projection is obviously a difficult problem and generally outside the
scope of folium, I agree.

On the other hand, the utility of not having to use cartopy, make a figure,
and save a temporary file for simple overlays of numpy arrays is pretty
huge for me (and others, I believe). The likelihood is high that anything
generated in numpy will be geodetic, so some projection needs to be done.

If geodetic_to_mercator is left in folium.utilities (and perhaps
referenced in the docstrings or docs?) I think this is an OK compromise. A
globally dataset is a common use case for generated data, and we account
for that. Anything else can be handled by the user.

ag

On Tue, Aug 18, 2015 at 10:11 AM Filipe notifications@github.com wrote:

I am re-thinking this issue and I believe it is better to avoid any
re-projection inside folium. Lets just say it takes the image in Web
Mercator and leave the transformation to the user. My notebook shows one
example of re-projecting Geodetic data to PlateCarree (which seems to be OK
in the map.)

We can easily create example with your geodetic_to_mercator() and rasterio
too. But I am leaning towards not hard-wiring this into folium.

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

[ocefpaf]

If `geodetic_to_mercator` is left in folium.utilities (and perhaps referenced in the docstrings or docs?) I think this is an OK compromise.

That makes sense. As soon as get back in the office I will change this PR to include the transformation back into utilities. We just need an example with globaly generated data for the docstring and I think I got one.

[ocefpaf]

We can try to use this in the global exemple:

http://scitools.org.uk/cartopy/docs/latest/examples/waves.html
Em 18/08/2015 11:42, "Filipe Pires Alvarenga Fernandes" ocefpaf@gmail.com
escreveu:

If geodetic_to_mercator is left in folium.utilities (and perhaps
referenced in the docstrings or docs?) I think this is an OK compromise.

That makes sense. As soon as get back in the office I will change this PR
to include the transformation back into utilities. We just need an example
with globaly generated data for the docstring and I think I got one.

[andrewgiessel]

Ya, good find. That example works well:

The latter is correct:

[andrewgiessel]

You can wrap data in a MPL colormap for fancy colors.

[ocefpaf]

OK. I am done with this PR. Here is an example that needs the projection step. The same example is illustrated in the docstrings.

Summary:

• Renamed the opacity option to image_opacity to be consistent with the rest of the API;
• The image_opacity is no transparency (1);
• No automatic projection is performed. The method assumes the image/data is in Web Mercator.
• There is an example and a utility function to project the data as Web Mercator (folium.utilities.geodetic_to_mercator)

@andrewgiessel can you take another look and give the 👍 or 👎

After that @BibMartin or @themiurgo can take another look and merge.

[andrewgiessel]

s/project/projected

[ocefpaf]

Thanks!

[andrewgiessel]

Some minor comments in the docstring, but 👍 from me! Great work and thanks for polishing this up so that it's more consistent with the whole project.

[ocefpaf]

One thing we need to come back when we pluginify this is the inline vs path reference. I think this should be a global option (standalone HTML vs separate files) and not inside the method.

[ocefpaf]

@BibMartin we have @andrewgiessel 👍 and green Travis. Can you take a quick look and merge?

[BibMartin]

It seems good. Congrats and thanks to both of you!