Add an argument to the compositor to switch alpha band on/off in DayNightCompositor #2358

yukaribbba · 2023-01-19T12:02:32Z

This PR is a solution for #2357. It only takes effect in "day only" or "night only" mode.

Closes Alpha band improvement for DayNightCompositor #2357
Tests added
Fully documented

codecov · 2023-01-19T12:16:30Z

Codecov Report

Merging #2358 (5127851) into main (ff7f237) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2358      +/-   ##
==========================================
+ Coverage   94.60%   94.64%   +0.03%     
==========================================
  Files         318      324       +6     
  Lines       47719    48337     +618     
==========================================
+ Hits        45146    45747     +601     
- Misses       2573     2590      +17

Flag	Coverage Δ
behaviourtests	`4.44% <2.12%> (-0.05%)`	⬇️
unittests	`95.27% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
satpy/composites/__init__.py	`90.51% <100.00%> (+0.06%)`	⬆️
satpy/tests/test_composites.py	`100.00% <100.00%> (ø)`
satpy/readers/netcdf_utils.py	`90.25% <0.00%> (-8.07%)`	⬇️
satpy/readers/viirs_sdr.py	`84.54% <0.00%> (-2.40%)`	⬇️
satpy/readers/yaml_reader.py	`97.26% <0.00%> (-0.23%)`	⬇️
satpy/readers/olci_nc.py	`93.81% <0.00%> (-0.16%)`	⬇️
satpy/tests/reader_tests/test_ghi_l1.py	`100.00% <0.00%> (ø)`
satpy/tests/reader_tests/test_nwcsaf_nc.py	`100.00% <0.00%> (ø)`
satpy/tests/reader_tests/test_fci_l1c_nc.py	`100.00% <0.00%> (ø)`
satpy/tests/reader_tests/test_viirs_atms_utils.py	`100.00% <0.00%> (ø)`
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

coveralls · 2023-01-19T14:06:02Z

Coverage: 95.23% (+0.03%) from 95.204% when pulling 5127851 on yukaribbba:main into ff7f237 on pytroll:main.

djhoese

I have one inline change to say include_alpha instead of need_alpha, but otherwise I have one other question/suggestion:

What if you forced the other data to be NaN in this case instead of 0 (like it is now)? This would mean you'd have to specify fill_value=0 on your save_datasets call, but I think this is more inline with other composites that Satpy produces...maybe? Put another way, the alpha band would be added during save_datasets rather than in the compositor.

satpy/composites/__init__.py

Co-authored-by: David Hoese <[email protected]>

yukaribbba · 2023-01-19T15:19:20Z

I have one inline change to say include_alpha instead of need_alpha, but otherwise I have one other question/suggestion:

What if you forced the other data to be NaN in this case instead of 0 (like it is now)? This would mean you'd have to specify fill_value=0 on your save_datasets call, but I think this is more inline with other composites that Satpy produces...maybe? Put another way, the alpha band would be added during save_datasets rather than in the compositor.

Yeah I tried that before but didn't work out so I turned to the compositor. It will force an alpha band added to the image and I think that's the reason. Only when it turned off the fill_value=0 has the effect.

djhoese · 2023-01-19T15:53:43Z

Yeah I tried that before but didn't work out so I turned to the compositor. It will force an alpha band added to the image and I think that's the reason. Only when it turned off the fill_value=0 has the effect.

Sorry I wasn't clear. I understand that the existing way the compositor work wasn't working as it was always adding an alpha band. What I'm suggesting is that in this compositor it currently replaces the masked data with 0 making it black. I think when this alpha flag is False that this should instead be np.nan so that the result of the compositor is still a single band, but the unwanted data is NaN instead of 0. This, by default, will produce an alpha band in the geotiff writer, but if you specify fill_value=0 in Scene.save_datasets it should become black. Since the compositor isn't creating the Alpha band this should work.

yukaribbba · 2023-01-19T16:18:31Z

Sorry I wasn't clear. I understand that the existing way the compositor work wasn't working as it was always adding an alpha band. What I'm suggesting is that in this compositor it currently replaces the masked data with 0 making it black. I think when this alpha flag is False that this should instead be np.nan so that the result of the compositor is still a single band, but the unwanted data is NaN instead of 0. This, by default, will produce an alpha band in the geotiff writer, but if you specify fill_value=0 in Scene.save_datasets it should become black. Since the compositor isn't creating the Alpha band this should work.

Sorry I got a little confused...

day_data = foreground_data if "day" in self.day_night else 0
night_data = foreground_data if "night" in self.day_night else 0

You're talking about these two lines? I could add another IF level for the False flag and make them np.nan. Is that correct? Not quite sure…

djhoese · 2023-01-19T16:21:06Z

Yeah I think that would work. It might also work to just replace 0 with np.nan since in the Alpha case those would be masked/transparent anyway.

yukaribbba · 2023-01-19T16:27:09Z

Yeah I think that would work. It might also work to just replace 0 with np.nan since in the Alpha case those would be masked/transparent anyway.

Got it. I’ll finish it tomorrow. Heading to bed now.😁

yukaribbba · 2023-01-20T03:11:13Z

Yeah I think that would work. It might also work to just replace 0 with np.nan since in the Alpha case those would be masked/transparent anyway.

I tried replacing 0 with np.nan in these two lines. However I always got both night and day portions masked-out despite other arguments. So I had to send it back to 0. I had several tests under the latest commit and it worked perfectly for me. You can see the upper left one is just what I need.

djhoese · 2023-01-20T03:18:38Z

Ah it is because the merging of the day and the night is a multiply and an addition:

satpy/satpy/composites/__init__.py

Line 750 in fc99734

data = night_portion + day_portion

Doing that with NaN is not going to work. Hhhmm I'm about to go to sleep. I'll have to think about this more.

yukaribbba · 2023-01-20T05:43:34Z

What about this? We send np.nan to coszen instead of the data itself, keeping the data at 0, and we can also make data = night_portion + day_portion safe.

yukaribbba · 2023-01-20T11:02:33Z

And the test results:

The last one is a little different from above. Since the night portion pixels are replaced with NaN, this area is considered as outter space. This is like what you said I suppose.

yukaribbba · 2023-01-20T14:24:41Z

Whoops…

djhoese · 2023-01-20T14:25:24Z

The difference you're talking about is how the bottom-right case doesn't "fade" into night time like the alpha=True cases, right? Yeah I think this makes sense. I'm not sure if it will be expected by all users but I think it is the only option if you don't want an alpha channel and also want only day(night) data.

@mraspaud @pnuu @gerritholl any opinions on this?

yukaribbba · 2023-01-20T14:28:45Z

I'm not sure if it will be expected by all users but I think it is the only option if you don't want an alpha channel and also want only day(night) data.

Yeah that’s what I want to say.

yukaribbba · 2023-01-23T11:48:39Z

Anything else I can do to this PR? @mraspaud @pnuu @gerritholl

mraspaud

LGTM

mraspaud · 2023-01-23T11:59:48Z

@yukaribbba thanks for this PR, it looks good to me. However, as @djhoese followed this more closely, I'd like to have his approval before merging this.

yukaribbba · 2023-01-23T12:05:12Z

@mraspaud Okay.😊

djhoese

Looks good. There is one issue with rendering the docs which I think I've fixed in my other comment. I'll commit that in a second.

Otherwise, my only other concern is something I think you've made the right decision on: the coszen is a value between 0 and 1 so for a day-only case you might want different things masked out. There is the night region, the twilight/mixed region, and the day region. If I say include_alpha=False do I want the twilight region removed or should it stay in the image? I think it needs to stay in the image which is what I think you've done here. This way it can be overlayed with a night-only image later on and there is no gap in the middle between the two images.

Right?

satpy/composites/__init__.py

yukaribbba · 2023-01-23T16:25:19Z

If I say include_alpha=False do I want the twilight region removed or should it stay in the image? I think it needs to stay in the image which is what I think you've done here. This way it can be overlayed with a night-only image later on and there is no gap in the middle between the two images.

Sure the twilight region will stay on the image. Here's a result for your case:

But, you may have noticed that this area seems to be a little darker since both images are fading there. So if you want a stacked image just simply use the default "day_and_night" and choose two normal composites.

djhoese · 2023-01-23T16:41:34Z

But, you may have noticed that this area seems to be a little darker since both images are fading there. So if you want a stacked image just simply use the default "day_and_night" and choose two normal composites.

Well see in my case (a possible future use, not currently doing this) would be having the individual masked day-only and night-only images and then in tiled image viewer (like https://geosphere.ssec.wisc.edu/) I would allow the user to switch the night image but keep the same day image. Generating all those combinations would not be a realistic option. I suppose what you're saying is that since these images would be "blending to black", they'll always have darker night-time transitions than they would if they were combined from the beginning (in satpy). Ya know, I'm OK with having the alpha channel in my images anyway so this probably isn't a realistic problem for me.

yukaribbba · 2023-01-23T16:50:12Z

Also, talking about this made me think of #2288 . Now we may have a easier solution.

true_color_corr:
lim_low: 85
lim_high: 88
day_night: day_only
include_alpha: false

true_color_uncorr:
lim_low: 85
lim_high: 88
day_night: night_only
include_alpha: false

true_color_mix:
- true_color_corr
- true_color_uncorr
lim_low: 85
lim_high: 88
day_night: day_night

Considering there could be some unexpected noises on night side, this could be cleaner:

true_color_mix_cleaner:
- true_color_mix
lim_low: 85
lim_high: 88
day_night: day_only
include_alpha: false

I have tested this recipe and it’s just perfect for me.

djhoese · 2023-01-23T16:51:29Z

CC @simonrp84 ^

yukaribbba · 2023-01-23T16:53:13Z

I suppose what you're saying is that since these images would be "blending to black", they'll always have darker night-time transitions than they would if they were combined from the beginning (in satpy).

Yep. That’s the point.

djhoese · 2023-01-23T16:54:41Z

Let's merge this and if someone comes up with other use cases that need changes to this implementation we can make additional PRs. Thanks @yukaribbba for putting this together and working with all my requests.

pnuu · 2023-01-23T16:55:52Z

Well see in my case (a possible future use, not currently doing this) would be having the individual masked day-only and night-only images and then in tiled image viewer (like https://geosphere.ssec.wisc.edu/) I would allow the user to switch the night image but keep the same day image. Generating all those combinations would not be a realistic option.

Oooh, I need to think about this idea, too!

yukaribbba · 2023-01-23T16:58:06Z

@djhoese @mraspaud Thanks for the help! 😁

gerritholl · 2024-11-29T10:29:36Z

See #3003 for a continuation of this discussion. I'm now trying to generate the day-only part of RGBs with the night part transparent but without an alpha layer. I can see a use case replacing the night part by black for RGBs mixing solar and terrestrial channels or ratios / differences between solar channels.

pnuu · 2024-11-29T10:33:13Z

Transparency won't happen without the alpha layer. The "other side" should be fill_value with include_alpha: False, though.

gerritholl · 2024-11-29T10:54:36Z

Pixels set to fill_value should be 100% transparent in supporting viewers.

pnuu · 2024-11-29T10:58:05Z

That depends also on the image format. But to be fully pedantic, the image won't have transparency :-)

Update __init__.py

d2bb5f8

yukaribbba requested review from djhoese and mraspaud as code owners January 19, 2023 12:02

Update __init__.py

f625c1c

yukaribbba added 2 commits January 19, 2023 20:38

Update test_composites.py

8fe4df0

Update test_composites.py

2e01455

Update composites.rst

c2090f5

yukaribbba requested a review from pnuu as a code owner January 19, 2023 14:58

djhoese requested changes Jan 19, 2023

View reviewed changes

satpy/composites/__init__.py Outdated Show resolved Hide resolved

Update satpy/composites/__init__.py

1256507

Co-authored-by: David Hoese <[email protected]>

yukaribbba added 2 commits January 19, 2023 23:24

Update __init__.py

0b31c18

Update composites.rst

7e47183

yukaribbba added 2 commits January 20, 2023 10:50

Update __init__.py

fc99734

Update test_composites.py

75dea01

Update __init__.py

68826ba

yukaribbba added 4 commits January 20, 2023 18:09

Update __init__.py

408a2b0

Update __init__.py

b415c95

Update composites.rst

9f0700f

Update test_composites.py

d40831b

yukaribbba requested review from djhoese and removed request for pnuu January 20, 2023 14:23

djhoese added enhancement code enhancements, features, improvements component:compositors labels Jan 20, 2023

Update composites.rst

2adcb21

mraspaud approved these changes Jan 23, 2023

View reviewed changes

mraspaud assigned yukaribbba Jan 23, 2023

djhoese approved these changes Jan 23, 2023

View reviewed changes

satpy/composites/__init__.py Outdated Show resolved Hide resolved

Fix docstring extra whitespace in satpy/composites/__init__.py

5127851

djhoese merged commit 955e301 into pytroll:main Jan 23, 2023

pnuu mentioned this pull request Feb 13, 2023

Fix gap between day/night-only composites #2388

Merged

3 tasks

gerritholl mentioned this pull request Nov 29, 2024

DayNightCompositor with day_only does not mask out night part if no alpha layer is present, defeating its purpose #3003

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an argument to the compositor to switch alpha band on/off in DayNightCompositor #2358

Add an argument to the compositor to switch alpha band on/off in DayNightCompositor #2358

yukaribbba commented Jan 19, 2023 •

edited

Loading

codecov bot commented Jan 19, 2023 •

edited

Loading

coveralls commented Jan 19, 2023 •

edited

Loading

djhoese left a comment

yukaribbba commented Jan 19, 2023

djhoese commented Jan 19, 2023

yukaribbba commented Jan 19, 2023

djhoese commented Jan 19, 2023

yukaribbba commented Jan 19, 2023

yukaribbba commented Jan 20, 2023

djhoese commented Jan 20, 2023

yukaribbba commented Jan 20, 2023 •

edited

Loading

yukaribbba commented Jan 20, 2023 •

edited

Loading

yukaribbba commented Jan 20, 2023

djhoese commented Jan 20, 2023

yukaribbba commented Jan 20, 2023

yukaribbba commented Jan 23, 2023

mraspaud left a comment

mraspaud commented Jan 23, 2023

yukaribbba commented Jan 23, 2023

djhoese left a comment

yukaribbba commented Jan 23, 2023

djhoese commented Jan 23, 2023

yukaribbba commented Jan 23, 2023

djhoese commented Jan 23, 2023

yukaribbba commented Jan 23, 2023

djhoese commented Jan 23, 2023

pnuu commented Jan 23, 2023

yukaribbba commented Jan 23, 2023

gerritholl commented Nov 29, 2024

pnuu commented Nov 29, 2024

gerritholl commented Nov 29, 2024

pnuu commented Nov 29, 2024

Add an argument to the compositor to switch alpha band on/off in DayNightCompositor #2358

Add an argument to the compositor to switch alpha band on/off in DayNightCompositor #2358

Conversation

yukaribbba commented Jan 19, 2023 • edited Loading

codecov bot commented Jan 19, 2023 • edited Loading

Codecov Report

coveralls commented Jan 19, 2023 • edited Loading

djhoese left a comment

Choose a reason for hiding this comment

yukaribbba commented Jan 19, 2023

djhoese commented Jan 19, 2023

yukaribbba commented Jan 19, 2023

djhoese commented Jan 19, 2023

yukaribbba commented Jan 19, 2023

yukaribbba commented Jan 20, 2023

djhoese commented Jan 20, 2023

yukaribbba commented Jan 20, 2023 • edited Loading

yukaribbba commented Jan 20, 2023 • edited Loading

yukaribbba commented Jan 20, 2023

djhoese commented Jan 20, 2023

yukaribbba commented Jan 20, 2023

yukaribbba commented Jan 23, 2023

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud commented Jan 23, 2023

yukaribbba commented Jan 23, 2023

djhoese left a comment

Choose a reason for hiding this comment

yukaribbba commented Jan 23, 2023

djhoese commented Jan 23, 2023

yukaribbba commented Jan 23, 2023

djhoese commented Jan 23, 2023

yukaribbba commented Jan 23, 2023

djhoese commented Jan 23, 2023

pnuu commented Jan 23, 2023

yukaribbba commented Jan 23, 2023

gerritholl commented Nov 29, 2024

pnuu commented Nov 29, 2024

gerritholl commented Nov 29, 2024

pnuu commented Nov 29, 2024

yukaribbba commented Jan 19, 2023 •

edited

Loading

codecov bot commented Jan 19, 2023 •

edited

Loading

coveralls commented Jan 19, 2023 •

edited

Loading

yukaribbba commented Jan 20, 2023 •

edited

Loading

yukaribbba commented Jan 20, 2023 •

edited

Loading