Add warning to default enhancement #3237
Conversation
djhoese
added
enhancement
There was a problem hiding this comment.
I like this, and I think it's great for user-defined RGBs, where the default enhancement is usually not what they want or should want. However, I'm a bit worried about the consequences for users looking at single-channel brightness temperatures or categorical data. If I understand this correctly, the following code will trigger the warning:
>>> sc.load([10.8])
>>> sc.show(10.8)So I think we should go ahead with this, but:
- The warning message should probably depend on whether this is single-channel or multi-channel data.
- We should have default enhancements for brightness temperature and perhaps some other common data.
- When the user is looking at data that do not need an enhancement, such as categorical data (
sc.show("cloud_mask")), I don't know if we need a warning at all.
satpy/enhancements/contrast.py
Outdated
| """Perform no enhancement but emit a warning. | ||
| The warning message is specified by the ``msg`` keyword argument | ||
| and can be a format string. Keyword arguments will be passed |
There was a problem hiding this comment.
There are at least seven ways to do string formatting in Python. The documentation should specify which type is meant. I think it's the type described as method 2b in the linked SO question.
There was a problem hiding this comment.
I think "format string" has a specific meaning and is similar to an f-string. That is, you call .format on it.
| formatted_msg = msg.format(**img.data.attrs) | ||
| warnings.warn(formatted_msg, UserWarning, stacklevel=2) | ||
| return img |
There was a problem hiding this comment.
I think we might want different warnings depending on the mode of the image data. If image data is mode L, chances are the user is simply doing sc.load([10.5]); sc.show([10.5]), or the equivalent for anything that's not a reflectance. For a quick look at a cloud mask, this is fine and a warning might needlessly scare users. But if the user has defined their own RGB and the output looks wrong, the warning might tell the user that they have not defined an enhancement or not correctly.
There was a problem hiding this comment.
I'm not sure I understand. Or maybe, I'm not sure how likely some of these scenarios are. A single band DataArray that is non-floating (category product) makes perfect sense and shouldn't be scaled, but should probably still have a warning that the user should configure an enhancement...
Oh or maybe there is a difference between a DEBUG log message and a UserWarning depending on how "reasonable" the case is. I guess the main deciding factor/problem is do we want the default/fallback enhancement to be something people can depend on (in specific cases) or should it always tell users they should define their own?
There was a problem hiding this comment.
My view:
If users are defining their own composite, they should always specify an enhancement. This can be their own or a built-in one from satpy. Not having an enhancement should be at least a warning, but possibly an error.
If users are loading single-channel datasets directly from a reader into a scene, there should be a reasonable default for sc.show(x), without a warning message.
Although we can not reliably tell the two cases apart (user-defined composites with SingleBandCompositor look similar to single-channel data), a substitute that works in many cases is to always warn for floating point RGBs without an enhancement defined, but to define defaults for single band floating point with common standard names. By far the most common example of single band floating point without an enhancement defined would be brightness temperature data. Maybe radiance too.
There was a problem hiding this comment.
Yes, of course, the BTs (and maybe radiances) need some logical well-accepted defaults. If we ignore that those are missing currently, I think we're still mostly saying the same thing. Here's what I think I'll try to implement today if I have time:
- Warn and stretch if float
- Debug log message if not float and no-op
The mixing of log and UserWarning here makes me think maybe I should also log a warning. I don't know.
|
This should be merged after a default BT enhancement is defined. |
pnuu
left a comment
pnuu
left a comment
There was a problem hiding this comment.
LGTM.
I was thinking that the douple check of floating point is a bit excessive, but I can't think of a better way to separate the two new functionalities.
|
I mean I could put them in one function but then I kept thinking someone might find them useful. |
|
Agreed. CI doesn't seem to be happy about ninjogeotiff tags. |
|
No it's that some test data doesn't have "name" defined and the warning message uses it. |
|
@pnuu @gerritholl What if in Satpy <1.0 I keep the existing warning, but don't stop scaling non-float data. I can also add an additional warning telling people of the change upcoming in Satpy 1.0. That change would be no longer scaling non-float data. |
|
I'm fine with that. |
Co-authored-by: Gerrit Holl <[email protected]>
Co-authored-by: Gerrit Holl <[email protected]>
Co-authored-by: Panu Lahtinen <[email protected]>
Warn if floating, debug log message otherwise
Warn about Satpy 1.0 changes
|
Ok I've started updating this PR again hoping to point @mraspaud at it. My last commit implements what I mentioned in my previous comment. That is, in Satpy <1.0 the behavior is the same but you get a warning about:
However, now I'm debating whether this change where non-floating point data is not going to be scaled. If we're warning people to make an explicit enhancement, then what's the point of careful/data-type-specific scaling anyway? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3237 +/- ##
=======================================
Coverage 96.30% 96.30%
=======================================
Files 463 463
Lines 58179 58194 +15
=======================================
+ Hits 56027 56042 +15
Misses 2152 2152
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 18761561777Details
💛 - Coveralls |
4 participants
Reviewers, I don't remember who was interested in this topic so forgive me if I missed anyone or added someone who doesn't care.
This PR changes the behavior of the default enhancement in Satpy. That is, the enhancement used when no other enhancement is configured. This PR adds a warning informing the user that there was no other enhancement configured. I should probably add more info about what the user should do about that.
I am also adding a no-op before falling back to the linear stretch if the data is integer type already as that it likely more useful (I think) than a dynamic stretch for data that is likely already image ready.
AUTHORS.mdif not there already