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 image overlay #152
Add image overlay #152
Conversation
@@ -16,6 +16,8 @@ | |||
import json | |||
from uuid import uuid4 | |||
|
|||
import imageio |
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 think its best to import this inside def image_overlay()
to keep this an "optional" dependency (same way we deal with pandas.)
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.
Sounds good - the reason the travis builds fail is because of this dependancy.
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.
There will be (probably) other failures that are unrelated. That is something I have on my plate to fix. Ignore them for now...
BTW add imageio
to our .travis.yml
!
@andrewgiessel nice work! I made a few inline comments. As a first pass we need:
|
Great! I agree with all of your comments, especially the min_ / max_ arguments- they are much more clear. I'll make those changes, and the 4 other things: docs, test, change.txt and travis.xml. When I run this locally, the generated HTML file doesn't have the have anything in the overlay layer. I'll look into it, but is there something we need add when the map is rendered to include the overlaid images? Edit: is master still 0.1.5? Should I add this to that section in CHANGES.txt? |
also- is there a typo here? Shouldn't we add data_layers not data_string? |
Ok, update. I realized the thing I needed to do was to edit the main html template! Derp. Then, I thought I needed the overlay with the other layers (which can work, but isn't needed and was hard to get it to turn on by default). So, I looked again at what you did in the html and realized it needed to go after the map creation call, and now it works. TODO still: docstring, test, changes, maybe readme.md (!). I did a small refactor of the data_layer string/template, but it doesn't seem to do anything and always seems empty. I can revert that edit if you want, but if you could look at The only major outstanding thing is the bug when you have a bit image and zoom a lot. I'm sure you'll see what I mean when you try it. It might not be worth fixing, and instead let upstream (leaflet) fix it. |
This is looking good! Thanks @andrewgiessel!! I am having some connection issues this, week so I cannot review this until next Friday. |
Yes, this looks great @andrewgiessel ! 👍 |
Thanks @ocefpaf + @themiurgo ! I am really happy with it too. The one thing I wanted to do was look into going from NxM to NxMx3 using a MPL colormap. might roll my own simple external lib. No problem re timing @ocefpaf. I'll hopefully have everything 'merge-ready' by then. |
Ah, just a little research into MPL means that this is already taken care of, and that we don't need to support anything fancy, because it's out of scope. 'image_overlay' will take a png, and that can be an NxM, NxMx3, NxMx4. For reference:
|
That's great! It would be great to integrate this into the docs, as it might be out of reach to those not familiar with matplotlib. Would you mind writing a few lines in the docs? If not that's totally cool and we'll take care of it some other time. Thanks again! |
@themiurgo you bet, writing the docstrings now. This is going to be a nice feature that is intermediate to a full blown WMS server. We should update README to highlight an example. |
Docstring updated. What version should this go under in CHANGES.txt? |
Ok, added a passing test! I think that the only thing left is to add something to CHANGES.TXT and perhaps readme.rst. The README looks like it could use a general refresh and so this might be very close indeed. |
Oddly, the test fails in travis, but not locally. Looking into it now. |
Ok, the travis build succeeds on the new test. The error is in fit_bounds - the dictionary is not ordered and thus the rendering happens in a random way. It passes on 2.7 and 3.4 but not on 3.3. Output:
Edit: this example is the working one. It fails when the order is off, obviously. |
One thought I had: does image_overlay to projection to web mercator, or spherical or whatever the basemap is in? |
hi @andrewgiessel ; that looks like great work ! I'm sorry for being late in the discussion. A few thoughts :
Again I'm late and sorry for this ; maybe I'll do another PR after your merge to integrate this. But I'd be interested in having your feeling about it. |
@BibMartin Thanks!
edit: i can look into this as the final commits of the PR, or we can add it as a feature after. |
As as aside, I mashed around enough last wee to get some tiles rendered to disk using I think this PR is a good quick and dirty solution, however, I would like feedback about how bad any projection artifacts might be. Also, any domain experts want to break down the options for tile servers? =) |
@ocefpaf @BibMartin @themiurgo Aside from the optional feature of doing embedded PNGs, the only thing I think that remains is the CHANGES.txt file. Let me know what I need to do to move forward! Thanks! |
I believe leaflet is always "web mercator". Unless...
Yep. I
I like Can you implement @BibMartin suggestions (2 and 3). If not please open issues for 2 and 3, so we do not lose track.
Thanks @themiurgo for the review! 🎉 Hurray community!!! 🎉 |
I agree ; let's finish up on both sides.
|
I wanted to share this gist here. It takes a numpy array, converts it to a byte array (which is really a string), then both encodes as a PNG. That buffer can be directly saved to disk or encoded in base64 for direct embedding in HTML https://gist.github.com/andrewgiessel/d5e1a11604af4aee5962 I'm about to use this to do the changes we talked about above, and then fix the build errors I caused by merging in upstream. At that point we should be good to merge. I'll make issues for any outstanding stuff, too. |
Great, thanks a lot! 😄 Once done with all the outstanding issues, please squash all the commits. If you're unsure about how to do it, please let us know and we'll guide you. |
I generally don't squash commits so if you have a URL that'd be handy, otherwise I'll google a bit. |
http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html tl;dr Identify the HEAD of the first, commit and then run |
@andrewgiessel one important thing to note is that you won't be able to |
DING. green! I am going to do some manual tests and then try and squash. |
@themiurgo sorry for the ignorance, but how do i find the HEAD of the first commit? I tried to use the hash of the first commit of the PR, but I think that is one too late (the pick/squash options started after that) do i need to look in the output of Just don't want to lose work here... and trying to get better at git =) EDIT: i think i found it. |
You can use |
88c348c
to
7ba63b9
Compare
I think I did it? pinging @ocefpaf @themiurgo
|
7ba63b9
to
25734fb
Compare
@andrewgiessel you did it! it would be great if you could change the commit message (shorten it to one line), to something that explains what the commit / PR is about. You can change the commit message by performing again a rebase (git rebase -i) and use the command |
Shoot!! @andrewgiessel Now you need to rebase against master (not your fault, we just merged some stuff). If you want to keep your git-fu road to mastery I can give you some pointer. Otherwise I can do that for you in a PR to your branch. |
@themiurgo I shortened it using @ocefpaf I'll try a rebase on master now... |
I checked out locally your branch, just in case something goes wrong. |
25734fb
to
b625613
Compare
... good? the places i had to change things for the master rebase were in .travis.yml and the tests. |
Everything seems good, it's a merge! 🎉 Thanks for the nice contribution @andrewgiessel ! |
Congrats @andrewgiessel, @themiurgo ; good contribution (and tricky git moves) |
🎉 🎉 🎉 👏 👏 👏 😎 😎 😎 |
This PR addresses #149.
As of submission, it is not complete. I think I am adding layers correctly and put in the right template form, but it is not adding the layer to the rendered map.
@ocefpaf What am I missing?