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
Added support for predefined colormaps with alpha channel and an example #161
Conversation
Thanks for the PR. We already have the functionality to provide a color map with alpha/transparency channel: You can provide the color map as 4D array: Doesn't that already fulfill your requirements? |
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.
just a note
tests/cmaps/test_get_cmap.py
Outdated
@@ -7,7 +7,7 @@ def test_get_cmap(): | |||
from terracotta.cmaps import get_cmap, AVAILABLE_CMAPS | |||
for name in AVAILABLE_CMAPS: | |||
cmap = get_cmap(name) | |||
assert cmap.shape == (255, 3) | |||
assert cmap.shape == (255, 3) or cmap.shape == (255, 4) |
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.
perhaps make that assert cmap.shape in [(255, 3), (255, 4)]
I haven't looked at the code yet, but here's my take. As I see it, the new functionality is a predefined color map that has an alpha gradient. So far, you had to specify your own color map to achieve this. Old version:
(assuming the raster consists of New version:
This is useful functionality IMO, but it also marks a break between our and matplotlib's color maps. At the least, we should generate a colormap reference (like this one) for our docs. An alternative would be an interpolated color map type, so you could do this like so:
which would interpolate between the colors, just like matplotlib's Happy for any opinions. |
@dionhaefner Yes! Your take is exactly correct, looking back at the code more thoroughly I may have only needed to make a change in the get_cmap method (line 41 in init.py) for the array shape assertion. I also really like the idea of allowing interpolated explicit colormaps. There stands a benefit for predefined cmaps to avoid recomputing the interpolation. @j08lue dionhaefner explained it better than me and provided clear examples, but add some number, if you have a color value for each of the 255 values and an average of 16 characters per color ( 10:[25,25,25,25] for example), you can easily end up with a json object just a little over 4kb in size going out with each request. I'm going to go back over the changes, and make sure I'm passing all of the tests before pushing again. I do apologize, I got a bit ahead of myself and jumped on pushing before it was ready. P.S. I love the work the team is doing and this is an amazing product. At work, we've used it with a couple projects now and I hope in doing so we've helped with some exposure to make it more well known. |
Maybe something like an environment variable |
And thank you for your kind words :) |
@brianpojo56 Do you as user have a suggestion how you could see yourself accessing your custom (4D) colormap on a deployed Terracotta instance? I very much understand that you do not want to pass the colormap data with every request. But how do you want to specify and provide it then? Would @dionhaefner's proposed solution work, which is:
Another solution would be to define additional color maps as JSON in an environment variable, e.g. Finally, you could also add your color map as a file to Terracottas In any case, we need to get the kind of changes in place that you proposed in this PR, so I'd encourage you to fix it up so the tests pass and we can take the discussion about deployment on the side. |
Let's leave the question of how to deploy cmaps to later. I am up for going through with the purpose of this PR (supporting 4D cmaps), provided that tests get fixed etc. first. What do you think @dionhaefner? |
Why don’t we just convert all colormaps to RGBA. That way we just need to support one file format. |
@j08lue @dionhaefner I'm really sorry I went MIA for so long, I had to make a short notice poorly planned road trip to see some family. There will be a separate folder for alpha maps. The benefits to this are: These are the only alpha maps that I can think of that may be useful: For the alphamap filenames I am very open to suggestions but I was thinking along the lines of these: If this all makes sense and sounds useful to you both I will have time this weekend I can devote to finishing this. |
Thanks for thinking more about this problem, but I'm afraid I can't agree with your proposal. We established that having an alpha gradient is useful in some cases, but it is and always will be a niche use case. That does IMO not warrant increasing the complexity of the API by introducing the alpha parameter. My counter proposal is this:
Unless anyone has any serious objections to this, this is how I would like to proceed. I would of course be happy if you @brianpojo56 want to contribute any of these pieces (some of them are already covered by this PR). |
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 this concisely implements the solution @dionhaefner proposed. Nice!
assert transparency_arr.shape == (256,) | ||
assert transparency_arr.dtype == 'uint8' | ||
transparency = transparency_arr.tobytes() |
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.
Yes, I guess these assert might as well apply to both, colormaps retrieved by name, and those user-defined.
Codecov Report
@@ Coverage Diff @@
## master #161 +/- ##
==========================================
+ Coverage 98.16% 98.16% +<.01%
==========================================
Files 43 43
Lines 2122 2123 +1
Branches 260 260
==========================================
+ Hits 2083 2084 +1
Misses 21 21
Partials 18 18
Continue to review full report at Codecov.
|
👋 @brianpojo56, thanks for the commits. Is this still a work in progress from your side or are you done with your contribution? |
@dionhaefner I had hoped to contribute a little more but it looks like I need to turn it over to you guys. The semester is starting to ramp up and I've lost my free time.
|
Nice overview of things to be done, @brianpojo56. I would be fine with merging now, which would fix your original issue, and leaving the other points to another PR. What do you think, @dionhaefner? |
Merged, I will continue in another PR. Thanks for the contribution @brianpojo56! |
The usecase that lead to this change is overlaying cloud coverage. See screenshot below. Cloud coverage information is extrapolated from GOES-16 ABI channel 13 and displayed on top of Bing Aerial Maps. Allowing a fourth channel in the predefined color maps allows us to have a varying transparency without sending a bulky json custom colormap with each api request. The varying transparency gives us the nice feathered edges for the clouds.