-
Notifications
You must be signed in to change notification settings - Fork 300
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 double alpha channel support and improve metadata behaviours for BackgroundCompositor #2696
Add double alpha channel support and improve metadata behaviours for BackgroundCompositor #2696
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2696 +/- ##
=======================================
Coverage 95.94% 95.94%
=======================================
Files 379 379
Lines 53673 53693 +20
=======================================
+ Hits 51494 51515 +21
+ Misses 2179 2178 -1
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 8762065249Details
💛 - Coveralls |
Could you describe the use case here? If the foreground has invalid pixels that you don't want to be filled in with background, why are you using the background compositor? Additionally, I believe there are "Filler" compositors that will fill in invalid data with a specific value. This could be done before calling the BackgroundCompositor. |
Well this is a little more complicated than I expected. I have to organize my thoughts. I think maybe I need a third channel used for masking area, just like there's a "high_resolution_band" in SharpenedCompositor |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
…b.com/yukaribbba/satpy into add_features_to_background_compositor
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 some comments
…b.com/yukaribbba/satpy into add_features_to_background_compositor
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.
LGTM! thanks for the simplifications
@djhoese rtd seems to fail, but I can't see any problem in the logs, any idea where I should look next? |
Right in the middle of the log:
|
duh, I was grep for warning... thanks |
Dang...This should be fine now. |
@yukaribbba shinx build now passes, but I'm not sure it looks like you intended, see here https://satpy--2696.org.readthedocs.build/en/2696/api/satpy.composites.html#satpy.composites.BackgroundCompositor |
damn, how can I do that? Is there something special like |
See here to find something you can use: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html |
Maybe a table would be better. Let's see how it renders. |
Heading to bed. I'll see this tomorrow. |
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.
LGTM!
Current BackgroundCompositor assumes that only foreground has an alpha band. What if both of them do? Like the newly added #2557 , both LowCloudCompositor and HighCloudCompositor generate images with alpha channels.
Here's an example of a
high_cloud
stacked on alow_cloud
:This is the result of current compositor. Apparently it can't handle the background alpha correctly.
Below is by this PR. It recognizes both alpha channels, and builds a new alpha.