Skip to content
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

Merged

Conversation

yukaribbba
Copy link
Contributor

@yukaribbba yukaribbba commented Dec 16, 2023

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 a low_cloud:
This is the result of current compositor. Apparently it can't handle the background alpha correctly.
image

Below is by this PR. It recognizes both alpha channels, and builds a new alpha.
image

Copy link

codecov bot commented Dec 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.94%. Comparing base (f5a8532) to head (05fdcbf).
Report is 1 commits behind head on main.

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     
Flag Coverage Δ
behaviourtests 4.09% <8.57%> (+<0.01%) ⬆️
unittests 96.04% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coveralls
Copy link

coveralls commented Dec 16, 2023

Pull Request Test Coverage Report for Build 8762065249

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 8732515933: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

@yukaribbba yukaribbba changed the title Add background_fill_in kwarg and double alpha channel support to BackgroundCompositor Add bg_fill_in kwarg and double alpha channel support to BackgroundCompositor Dec 16, 2023
@djhoese
Copy link
Member

djhoese commented Dec 16, 2023

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.

@yukaribbba
Copy link
Contributor Author

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

@yukaribbba yukaribbba changed the title Add bg_fill_in kwarg and double alpha channel support to BackgroundCompositor Add a third optional dataset for masking and double alpha channel support to BackgroundCompositor Dec 18, 2023
@yukaribbba
Copy link
Contributor Author

pre-commit.ci autofix

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

just some comments

satpy/composites/__init__.py Outdated Show resolved Hide resolved
satpy/composites/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@mraspaud mraspaud left a 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

@mraspaud
Copy link
Member

@djhoese rtd seems to fail, but I can't see any problem in the logs, any idea where I should look next?

@djhoese
Copy link
Member

djhoese commented Apr 19, 2024

Right in the middle of the log:


/home/docs/checkouts/readthedocs.org/user_builds/satpy/conda/2696/lib/python3.10/site-packages/satpy/composites/__init__.py:docstring of satpy.composites.BackgroundCompositor:5: ERROR: Unexpected indentation.

@mraspaud
Copy link
Member

Right in the middle of the log:


/home/docs/checkouts/readthedocs.org/user_builds/satpy/conda/2696/lib/python3.10/site-packages/satpy/composites/__init__.py:docstring of satpy.composites.BackgroundCompositor:5: ERROR: Unexpected indentation.

duh, I was grep for warning... thanks
@yukaribbba the identation of the docstring doesn't seem to agree with sphinx...

@yukaribbba
Copy link
Contributor Author

Right in the middle of the log:


/home/docs/checkouts/readthedocs.org/user_builds/satpy/conda/2696/lib/python3.10/site-packages/satpy/composites/__init__.py:docstring of satpy.composites.BackgroundCompositor:5: ERROR: Unexpected indentation.

duh, I was grep for warning... thanks @yukaribbba the identation of the docstring doesn't seem to agree with sphinx...

Dang...This should be fine now.

@mraspaud
Copy link
Member

@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

@yukaribbba
Copy link
Contributor Author

damn, how can I do that? Is there something special like \n that rtd can recogonize?

@mraspaud
Copy link
Member

See here to find something you can use: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html
Maybe a bullet list?

@yukaribbba
Copy link
Contributor Author

yukaribbba commented Apr 19, 2024

Maybe a table would be better. Let's see how it renders.

@yukaribbba
Copy link
Contributor Author

Heading to bed. I'll see this tomorrow.

@yukaribbba
Copy link
Contributor Author

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM!

@mraspaud mraspaud merged commit 211cda2 into pytroll:main Apr 22, 2024
18 of 19 checks passed
@yukaribbba yukaribbba deleted the add_features_to_background_compositor branch April 22, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component:compositors enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvements for BackgroundCompositor
5 participants