Add "neutral_resolution_band" kwarg to RatioSharpenedRGB/SelfSharpenedRGB

[yukaribbba]

Current RatioSharpenedRGB/SelfSharpenedRGB compositor is based on one assumption: there's only one high-res band. However in some composites, like 'false_red_color' of VIIRS, there could be a second high-res band. And it will be wrongfully sharpened in current compositor. So this PR will fix it by adding a new kwarg called "neutral_resolution_band".

Here's a comparison.

The current one
R: I02 -- 375m -- another high-res band (no need for sharpening but still got it)
G: M05/I01 -- 750m/375m -- high-res band (calculate ratio)
B: M04 -- 750m -- low-res band (need sharpening)

  false_red_color:
    compositor: !!python/name:satpy.composites.RatioSharpenedRGB
    prerequisites:
    - name: I02
      modifiers: [sunz_corrected_iband]
    - name: M05
      modifiers: [sunz_corrected, rayleigh_corrected]
    - name: M04
      modifiers: [sunz_corrected, rayleigh_corrected]
    optional_prerequisites:
    - name: I01
      modifiers: [sunz_corrected_iband, rayleigh_corrected_iband]
    standard_name: crefl
    high_resolution_band: green

(100% enlarged)

(100% enlarged -- I02)

This PR
R: I02 -- 375m -- another high-res band (marked as "neutral_resolution_band" so won't get involved)
G: M05/I01 -- 750m/375m -- high-res band (calculate ratio)
B: M15 -- 750m -- low-res band (need sharpening)

  false_red_color:
    compositor: !!python/name:satpy.composites.RatioSharpenedRGB
    prerequisites:
    - name: I02
      modifiers: [sunz_corrected_iband]
    - name: M05
      modifiers: [sunz_corrected, rayleigh_corrected]
    - name: M04
      modifiers: [sunz_corrected, rayleigh_corrected]
    optional_prerequisites:
    - name: I01
      modifiers: [sunz_corrected_iband, rayleigh_corrected_iband]
    standard_name: crefl
    high_resolution_band: green
    neutral_resolution_band: red

(100% enlarged)

(100% enlarged -- I02)

Additionally, if there's no high-resolution file provided, a non-sharpened, low-res image will come out instead.
R: I02 -- 375m
G: M05 -- 750m
B: M15 -- 750m

  false_red_color:
    compositor: !!python/name:satpy.composites.RatioSharpenedRGB
    prerequisites:
    - name: I02
      modifiers: [sunz_corrected_iband]
    - name: M05
      modifiers: [sunz_corrected, rayleigh_corrected]
    - name: M04
      modifiers: [sunz_corrected, rayleigh_corrected]
    optional_prerequisites:
    - name: I01
      modifiers: [sunz_corrected_iband, rayleigh_corrected_iband]
    standard_name: crefl
    high_resolution_band: green
    neutral_resolution_band: red

Note that "neutral_resolution_band" should be considered as a complementary to "high_resolution_band". So only when the high-resolution file is given and "high_resolution_band" is set, this kwarg takes effect.

• Tests added
• Fully documented

[djhoese]

This is also the case for some true/false/natural color RGBs with ABI or MODIS or VIIRS so it is definitely common. However, I don't think I needed to use it because of the ways I was separating the bands in my own processing. I'm OK with this feature if you get the tests passing.

Should the value be a list to specify multiple bands? I suppose the only case where that would be needed is if this compositor also supports RGBA generation with 4 bands as inputs.

[yukaribbba]

However, I don't think I needed to use it because of the ways I was separating the bands in my own processing.

Yeah I also did it that way but sometimes I still need this for a quicklook. Well if this compositor can produce RGBA images then the kwarg should be a list. But I haven't found such one so I think I'll leave it for future. For now this is just a minor update.

I can't get through the test but it looks like not my fault. It seems other people's PRs also have the same errors.

FAILED satpy/tests/scene_tests/test_conversions.py::TestSceneConversions::test_geoviews_basic_with_area - ImportError: cannot import name 'LinkCallback' from 'holoviews.plotting.bokeh.callbacks' (/usr/share/miniconda3/envs/test-environment/lib/python3.11/site-packages/holoviews/plotting/bokeh/callbacks.py)
FAILED satpy/tests/scene_tests/test_conversions.py::TestSceneConversions::test_geoviews_basic_with_swath - ImportError: cannot import name 'LinkCallback' from 'holoviews.plotting.bokeh.callbacks' (/usr/share/miniconda3/envs/test-environment/lib/python3.11/site-packages/holoviews/plotting/bokeh/callbacks.py)
FAILED satpy/tests/writer_tests/test_cf.py::TestEncodingKwarg::test_encoding_kwarg[True] - assert 0 == 7
FAILED satpy/tests/writer_tests/test_cf.py::TestEncodingAttribute::test_encoding_kwarg[True] - assert 0 == 7
FAILED satpy/tests/writer_tests/test_cf.py::TestEncodingAttribute::test_encoding_attribute[True] - assert 0 == 7

[djhoese]

I just merged a PR that fixed the issues with CI. Try rebasing your main with the pytroll main (or merging) and things should start passing. In the future I highly recommend making a new branch specific to the PR you're going to make. Using main makes it difficult for cases like this where you need to synchronize with upstream main.

[yukaribbba]

Got it.

[codecov]

Codecov Report

Merging #2489 (7a03751) into main (1f97c0b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2489   +/-   ##
=======================================
  Coverage   94.84%   94.85%           
=======================================
  Files         337      337           
  Lines       49571    49624   +53     
=======================================
+ Hits        47016    47069   +53     
  Misses       2555     2555           

Flag	Coverage Δ
behaviourtests	4.41% <1.78%> (-0.01%)	⬇️
unittests	95.47% <100.00%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
satpy/composites/__init__.py	91.23% <100.00%> (+0.03%)	⬆️
satpy/tests/test_composites.py	100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

[coveralls]

Pull Request Test Coverage Report for Build 5245550960

• 54 of 54 (100.0%) changed or added relevant lines in 2 files are covered.
• 17 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.005%) to 95.419%

---

Files with Coverage Reduction	New Missed Lines	%
satpy/readers/avhrr_l1b_gaclac.py	6	95.68%
satpy/readers/olci_nc.py	11	94.2%

Totals
Change from base Build 5141473420:	0.005%
Covered Lines:	47182
Relevant Lines:	49447

---

💛 - Coveralls

[djhoese]

Thanks for putting this together. I had a lot of comments and things I'm unsure of that I commented inline. Another thought I had while reviewing this though is that it takes away from the automatic choosing/handling of the cases where the high resolution band isn't available. So for example in the VIIRS case if you didn't provide the I-band resolution data files then asking for a ratio sharpened true color would just get you the low resolution version. With this neutral resolution functionality, maybe it would be best if there were two optional datasets available: the high resolution band used for ratios and the neutral one used inplace of the low resolution option. Hhhmmm but that makes it ugly in cases where there is no low resolution version of the "neutral" band.

For now I'll post this review that is likely very confusing and I'll have to think about these things offline. The functionality is good, but I think there are cleaner ways to handle some of the logic.

[yukaribbba]

Another thought I had while reviewing this though is that it takes away from the automatic choosing/handling of the cases where the high resolution band isn't available.

Well the "neutral" block is completely under "high res", so if you don't give any high res files it will produce a low res image, just like current one. I've updated the examples above so you can see that case.
I want this PR have minor impact on the existed script/yaml files. So having two "optional datasets" could be confusing for some users....

[djhoese]

pre-commit.ci autofix

[yukaribbba]

Ready

[djhoese]

I had some questions about some of the tests and one more request on cleaning up the code to make CodeScene a little happier. It also seems like pre-commit wasn't run so pre-commit.ci is complaining about some small things. I can fix those after the other changes are made though.

[yukaribbba]

pre-commit.ci autofix

[yukaribbba]

pre-commit.ci autofix

[yukaribbba]

CI keeps failing in coveralls stage.

[djhoese]

I think coveralls was just having a hard time. I've restarted the jobs and we'll see how things go now.

[djhoese]

I think this looks good to me. Thanks! And thanks for sticking with it with all of the changes I asked for. I'll merge this now, but maybe we should have another PR where we change Satpy's builtins to use this? What existing ones would benefit from this functionality?