-
Notifications
You must be signed in to change notification settings - Fork 303
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
Implement support to set alpha range in create_colormap and yaml colorize enhancements #2817
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2817 +/- ##
=======================================
Coverage 95.94% 95.94%
=======================================
Files 366 366
Lines 53561 53611 +50
=======================================
+ Hits 51389 51439 +50
Misses 2172 2172
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 9500484605Details
💛 - Coveralls |
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.
The colormap logic was actually moved to trollimage a while ago (by @gerritholl I think) but I don't think we ever migrated Satpy to use the trollimage function or required satpy to use the newer version of trollimage. I think this PR should maybe be migrated to trollimage and a separate PR be setup to switch to requiring the version of trollimage that introduced this logic. Sorry I don't have more details about what changed and what should be changed. It's been a while since I've looked at this.
Hi Dave, thanks for the review! Indeed, at the beginning I thought of this only as an improved interface from satpy to the So I created a new method in trollimage: pytroll/trollimage#170 I updated this PR accordingly, but kept the tests for the new functionality here, and and of course they're failing now since we need the trollimage PR released first. |
After the released update in pytroll/trollimage#170, and the according change in the required trollimage version, the tests related to this PR are passing again as expected, so from that point of view I believe this PR is ready. However, now there are failures on the |
The SAR tests broke because of backwards incompatibility introduced in rioxarray release 0.15.6 and in particular this PR corteva/rioxarray#787 |
Pull Request Test Coverage Report for Build 9647136913Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Tests passing again after blacklisting rioxarray 0.15.6 |
Pull Request Test Coverage Report for Build 9694281199Details
💛 - Coveralls |
I reverted the rioxarray blacklist after Martin's fix, and tests pass again now after Dave's fixes yesterday. So from my side this is good to go again. |
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.
Looks pretty good. Just a few small requests.
Pull Request Test Coverage Report for Build 9743559474Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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.
Looks good to me. I'm not in love with color_scale =
being accessed outside of _get_cmap_from_palette_info
as that information is part of the palette, but I don't see an obvious better solution given what you need to do.
If we can get one other maintainers review that'd be great, but if no one is available in the next day or so I can merge this for you so you can continue your work.
This PR adds the possibility to define an alpha channel range to be applied to a colorise colormap, e.g. through the yaml enhancement configuration.
With this, composites like the one below are more easily achievable, with e.g.