Skip to content

Conversation

jpn--
Copy link
Contributor

@jpn-- jpn-- commented Dec 7, 2019

This addresses #89. Tile sets identified in providers that are known to be overlay tiles, with an embedded alpha channel, using the same named rules from leaflet) or that have defined opacity less than 1.0 are drawn over other data (with a default zorder of 9, unless this is overridden manually). This will typically put the overlay tiles on top of other baseman tiles (almost certainly always good) and also on top of other data being plotted by the user. This second outcome is sometimes good, sometimes not, and I'm not sure if this default setting is reasonable. But, even setting zorder to 1 will put the labels on top of other user content unless the user intentionally put a zorder value on what they draw. I am certainly open to implementing other options here if anyone cares deeply (or shallowly).

One side effect of this change is that all map tiles are processed with the alpha channel intact. This will add a bit to memory requirements, but likely not enough for anyone to notice (other than the tests, which I've edited to accommodate the alpha channels across the board.)

Tilesets identified in providers that are known to be overlay tiles (with an embedded alpha channel) or that have defined opacity less than 1.0 are drawn over other data (with a default zorder of 9, unless this is overridden manually).
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 92.857% when pulling c663965 on jpn--:overlaytiles into 3cce6d7 on darribas:master.

@jpn--
Copy link
Contributor Author

jpn-- commented Dec 7, 2019

Having the default zorder as noted above lets you do this, putting labels over both the background and your data, without worrying about the order of commands to put it together:

image

@JSybrandt
Copy link

Any idea if/when a release is going to get this feature?

@darribas
Copy link
Collaborator

darribas commented Mar 3, 2020

Thanks for this @jpn-- !!! I haven't had a second to review it myself, but does not look to onerous. Right now, our efforts are centered on settling the providers api, which we already have decided what to do (#90) and with that I'd be keen to push 1.0 this spring. If this is not too onerous and we can get it reviewed, I think it'd be a very cool feature to have!

…iles

Updates to test_ctx.py for compatibility with recent changes to ctx
@jpn--
Copy link
Contributor Author

jpn-- commented Apr 16, 2020

I noticed development here recently kicked into "active", and that some of the recent changes wouldn't merge cleanly with the changes I made to the tests for the overlay feature, so I went ahead and tweaked my changes so that they do merge cleanly, hopefully this makes it easier to review this now. Thanks to you all for making this awesome tool!

@jpn-- jpn-- marked this pull request as draft April 24, 2020 21:28
@jpn-- jpn-- marked this pull request as ready for review April 24, 2020 22:04
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

And thank you for this contribution!

Generally looks good to me, added some minor comments.
We probably want to add some examples for this in the notebook about add_basemap, once that exists (so don't worry about that for now)

@jpn--
Copy link
Contributor Author

jpn-- commented Apr 25, 2020

We probably want to add some examples for this in the notebook about add_basemap, once that exists (so don't worry about that for now)

Glad to contribute! When the rest of a notebook about add_basemap exists, if you want me to circle back and put in an example, tag me and I can help with that too.

jpn-- added 3 commits January 8, 2021 16:23
…iles

* commit '3c26d5a16a094e8d5ae05f8e5a41f05278816daf':
  DOC: update for latest pydata-sphinx-theme (fix sidebar + use CSS variables) (geopandas#168)
  RLS: v1.0.1
  Fix resetting of extent with local files (geopandas#155) (geopandas#156)
  Add random user ID for Nominatim and allow arbitrary geocoder (geopandas#164)
  Close and reopen memfile dataset before WarpedVRT (geopandas#165)
  Fix use of rasterio MemoryFile (geopandas#163)
  DOC: Use Cape Town consistently in description and variable names (geopandas#154)
  Add matplotlib Framework classifier (geopandas#152)
  Require Python 3.6 as minimum version (geopandas#150)
  Add earthengine to docker for binder
  Example interfacing GEE with contextily (geopandas#147)
  DOC: add notebook about working with local files (geopandas#139)

# Conflicts:
#	tests/test_ctx.py
@jpn--
Copy link
Contributor Author

jpn-- commented Jan 9, 2021

I needed to use this so I updated it for myself. I also added an example in the add_basemap notebook.

@darribas
Copy link
Collaborator

This looks fantastic! @jorisvandenbossche do the changes look good to you? If so, we can merge, and I think it'd be good to push it as 1.1, it's a really cool little feature! (I'm happy to take care of the release)

@danielsclint
Copy link

This feature is really cool and nice to have. I stumbled on a need for this, and @jpn-- patch got me fixed up right away.

@darribas
Copy link
Collaborator

Hello everyone, sorry for my slow moving on this (my bandwidth is what it is...). Just to say I'm planning on having a look at this PR in the coming days and if it's all good, happy to merge and have this on contextily. Once merged, I think this is good enough to cut a 1.1 release so folks can enjoy it more widely. Hopefully more on this soon...

@darribas
Copy link
Collaborator

Hi @jpn-- one quick thing, if I checkout your PR and try to open the intro notebook, I get this error:

[W 03:29:58.793 NotebookApp] Unreadable Notebook: /home/jovyan/work/notebooks/intro_guide.ipynb NotJSONError('Notebook does not appear to be JSON: \'{\\n "cells": [\\n  {\\n   "cell_type": "m...')

Can you have a look to see if there's any conflict of some sort? Thanks! I'm looking at the other files too :)

@jpn--
Copy link
Contributor Author

jpn-- commented Jan 27, 2021

Sorry @darribas, my experiment with commit hooks was too clever by half, or not clever enough? In any case, the notebook should be all better now.

Copy link
Collaborator

@darribas darribas left a comment

Choose a reason for hiding this comment

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

This is fantastic!!! I've had a more in detail look to get my hear around and this is so simple yet so powerful! If you consider this good to go, I'm very happy to merge it, just OK this comment

@jpn--
Copy link
Contributor Author

jpn-- commented Jan 27, 2021

I'm not clear if "you" is me or @jorisvandenbossche, I'm happy to see this merged either way.

@darribas darribas merged commit 99b2e05 into geopandas:master Jan 28, 2021
@darribas
Copy link
Collaborator

Just wanted to make sure there wasn't any extra add to the PR. THANK you very much for the contribution this is fantastic! Hopefully I can cut a quick release in the coming days so it's available on pip/conda to everyone!

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.

6 participants