-
Notifications
You must be signed in to change notification settings - Fork 293
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 a blend method to create temporal RGB from MultiScene #2488
Add a blend method to create temporal RGB from MultiScene #2488
Conversation
Nice start. But rather specific to the use case of a T-2/T-1/T-0 RGB. Would be interested in a discussion on how we can set up a more generic way of configuring this in YAML, where this would be just one specific case. |
Indeed, this is very specific to one of the cases, so didn't mark this to close the original issue. I think the YAML variant would need to use
|
It looks like that if I have a |
Yes, that's by design and tested. We have only three channels in any way 😉 |
Unless we have some way for the
This would be somehow found by Advantage: we can use modifiers and enhancements the same way we do for other composites — and we could replace The weakness: I don't know how the "somehow" would work. A Scene couldn't load this composite, because a Scene has only one timestep. In your example, how would modifiers and enhancements work? |
Simply by defining a (single-channel) composite for a given data: hrv:
compositor: !!python/name:satpy.composites.GenericCompositor
prerequisites:
- name: HRV
modifiers: [sunz_corrected]
standard_name: hrv And to complement, an enhancement: hrv:
standard_name: hrv
operations:
- name: stretch0
method: *stretchfun
kwargs:
stretch: crude
min_stretch: 0
max_stretch: 100 And now instead of doing |
That's the point where we'd need to do some |
Why? The MultiScene already has the scenes for each time step, provided by the user directly or (for operations) by a multi-timestep gatherer in pytroll-collectors. |
I approve of this PR as-is, but understand that it doesn't fully satisfy all of the use cases that some people had in mind. As you've pointed out the MultiScene doesn't really have a concept or understanding of what the Scenes are that are contained within it. It doesn't know if they're multiple orbits, multiple timesteps/repeat cycles, different satellites/sensors, etc. So there will always have to be some level of understanding by the user that "functionality X only works in these specific cases of the MultiScene". At least as currently designed. One additional thing to consider is using the blend method with |
Co-authored-by: David Hoese <[email protected]>
True, but the composite loading happens at individual |
compositor = GenericCompositor("temporal_composite") | ||
composite = compositor((data_arrays[0], data_arrays[1], data_arrays[2])) |
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.
If there is time metadata available, this would probably fail with IncompatibleTimes
, due to the check in composites.check_times
.
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.
Any idea which data has that so I could test? SEVIRI HRIT and AVHRR L1b AAPP worked.
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 in a lot of cases we started removing the time metadata in the readers because it was so inconsistent. I could be completely misremembering though.
Codecov Report
@@ Coverage Diff @@
## main #2488 +/- ##
=======================================
Coverage 94.83% 94.83%
=======================================
Files 337 337
Lines 49430 49461 +31
=======================================
+ Hits 46875 46906 +31
Misses 2555 2555
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Pull Request Test Coverage Report for Build 5077181999
💛 - Coveralls |
""" | ||
from satpy.composites import GenericCompositor | ||
|
||
compositor = GenericCompositor("temporal_composite") |
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.
GenericCompositor
removes units and calibration information, which we probably don't want if we produce an RGB from three times the same channel. Maybe GenericCompositor
contains other (implicit) assumptions that make it not optimal for temporal composites.
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, good point. GenericCompositor is really "generic image compositor" so it assumes things no longer represent "real physical data" anymore.
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.
Units don't make sense for the returned RGB, so that's fine.
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.
Why would units not make sense if I combine three times HRV or three times 10.8 µm?
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.
As this is creating an RGB, we are already implicitely renaming the time dimension with bands
, so I get we already moved out of the scientific realm to move into the imaging one.
Apparently the data should be from times T/T-1/T not T-2/T-1/T, but the code don't care so that's possible as long as the correct times are used. |
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!
In #1748 we've discussed composites with temporal dependency. This is one attempt on one of the ideas.