Make sure RGBs do not have units attributes. #2068

gerritholl · 2022-03-22T10:21:16Z

As per the discussion in #2066, make sure that RGBs do not have units attributes.

Also in this PR:

Refactor VIIRS unit tests
Add unit test for VIIRS snow age RGB
Add or extend unit tests for false color, natural color, ocean color, true color, and true color crefl, including tests for absence of RGBs
Closes RGBs should never have units, but some do #2066
Tests added
Fully documented

Does not change the ssec_fog composite, because the result of the DifferenceCompositor is still a quantity and should correctly have units.

For the VIIRS composites unit tests, refactor common code into fixtures.

Add a unit test for the snow age RGB. This unit test currently fails, because it confirms, among other things, that there there is no units attribute present. Also add a reference to the snow age publication.

Add a test to confirm that RGBs produced by the RatioSharpenedRGB have no units attribute.

In the SnowAge and RatioSharpened compositors, clear the units attribute after the RGB has been produced.

gerritholl · 2022-03-22T10:41:04Z

I don't see how the test failures are related to anything I did. The tests, including pre-commit linting tests, pass on my local machine, and the failures in GitHub CI are in an entirely different part of the codebase.

djhoese · 2022-03-22T11:56:40Z

Don't all of these composites use the GenericCompositor? I think, despite the name being misleading, that class is used for all multi-band image products including the DNB compositors, right? Could this unit pop be done in that compositor or is that too dangerous?

gerritholl · 2022-03-22T13:06:58Z

They do use the GenericCompositor. The class hierarchy for the compositors defined in composites.__init__:

        CompositeBase
            CategoricalDataCompositor
            DifferenceCompositor
            GenericCompositor
                BackgroundCompositor
                CloudCompositor
                ColormapCompositor
                    ColorizeCompositor
                    PaletteCompositor
                DayNightCompositor
                Filler
                FillingCompositor
                LongitudeMaskingCompositor
                LuminanceSharpeningCompositor
                MaskingCompositor
                MultiFiller
                NaturalEnh
                RGBCompositor
                RatioSharpenedRGB
                    SelfSharpenedRGB
                RealisticColors
                SandwichCompositor
                StaticImageCompositor(GenericCompositor, satpy.aux_download.DataDownloadMixin)
            SingleBandCompositor

and the ones in viirs.py:

    satpy.composites.CompositeBase(builtins.object)
        ERFDNB
        HistogramDNB
            AdaptiveDNB
        NCCZinke
    satpy.composites.GenericCompositor(satpy.composites.CompositeBase)
        SnowAge

The ones in abi.py, abi.py, cloud_products.py, glm.py, and sar.py all derive directly or indirectly from GenericCompositor.

One class I'm not sure about here is the MaskingCompositor. This returns (normally) an LA composite, where units can be reasonably interpreted as belonging to the L layer only. It might be that retaining the units could be desirable in this case, so doing the pop in GenericCompositor may be a little risky.

NB: I'm not sure why MaskingCompositor derives from GenericCompositor, but that's a different question.

One other alternative would be that GenericCompositor gets a flag that tells whether units should be retained or not, but that makes composite configuration a bit more complicated.

pnuu · 2022-03-22T13:17:24Z

NB: I'm not sure why MaskingCompositor derives from GenericCompositor, but that's a different question.

Plainly because it handles all the different modes. I'm more surprised that not all compositors use it as the base.

djhoese · 2022-03-22T13:55:17Z

I'm more surprised that not all compositors use it as the base.

The main reason not to use GenericCompositor as the base is for those cases where the composite is not an "image" and is meant to represent some kind of realistic data observation or manipulation (temperature differences, etc).

@gerritholl Ok, so it seems we have cases where units on an L image may make sense (at least until the composite is enhanced). I'm ok with doing the individual pops in the specific compositors, but there are probably other composites that will still have units if they consist of bands of the same type. I think airmass combines reflectances and BTs, right? Composites like that though with only reflectances as inputs or only BTs would still have the units.

gerritholl · 2022-03-22T14:01:40Z

there are probably other composites that will still have units

Perhaps. It's not so easy to be sure, without comprehensively covering all composites, for which I would need data I don't have. Most unit tests don't identify the issue, because they don't add a units attribute to the synthetic test data in the first place.

gerritholl · 2022-03-22T14:07:22Z

I could do the popping in GenericCompositor, but on the condition that there are three or more bands. That would be a more generic solution, but I would still be not entirely sure that I wouldn't miss a case in the unit tests. Would you prefer to see it in GenericCompositor?

djhoese · 2022-03-22T15:14:07Z

I say we wait for @mraspaud to get back and see if he or other @pytroll/satpy-core maintainers have an opinion. This almost sounds like we need another compositor class to handle the Masking for image-like data versus band-like data. Or maybe there is a keyword argument or instance attribute or class attribute that controls the behavior of the "metadata filtering" and when it is applied.

gerritholl · 2022-03-23T17:12:45Z

What can/should I do about

@codefactor-io codefactor-io / CodeFactor
satpy/tests/test_composites.py#L0
Complex Code

Do we need to split the whole module into multiple modules? The test methods are already factored into classes, so I'm not sure what further module-level refactoring to do. I've only added one method to a class that had 8 methods, now 9. That does not seem excessive.

djhoese · 2022-03-30T18:41:24Z

I think overall we need to get composites out of composites/__init__.py and into separate modules under composites/. The same could/should apply to the tests. Lots of work and decisions to be made, but that's probably the way forward. I personally don't like lots of code in a __init__.py module anymore.

gerritholl · 2022-03-31T07:34:31Z

I think overall we need to get composites out of composites/__init__.py and into separate modules under composites/. The same could/should apply to the tests. Lots of work and decisions to be made, but that's probably the way forward.

Can we defer that to a later PR?

djhoese · 2022-03-31T13:38:17Z

I'd be ok with that.

codecov · 2022-04-06T09:05:40Z

Codecov Report

Merging #2068 (72f3acb) into main (fd483d3) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2068      +/-   ##
==========================================
+ Coverage   93.79%   93.84%   +0.04%     
==========================================
  Files         283      283              
  Lines       42284    42276       -8     
==========================================
+ Hits        39662    39672      +10     
+ Misses       2622     2604      -18

Flag	Coverage Δ
behaviourtests	`4.74% <0.00%> (+<0.01%)`	⬆️
unittests	`94.40% <100.00%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
satpy/composites/__init__.py	`89.94% <100.00%> (+0.02%)`	⬆️
satpy/composites/viirs.py	`94.69% <100.00%> (+4.82%)`	⬆️
satpy/tests/compositor_tests/test_viirs.py	`100.00% <100.00%> (ø)`
satpy/tests/test_composites.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd483d3...72f3acb. Read the comment docs.

coveralls · 2022-04-06T09:48:39Z

Coverage increased (+0.04%) to 94.342% when pulling 72f3acb on gerritholl:no-units-for-rgbs into fd483d3 on pytroll:main.

mraspaud

LGTM

mraspaud · 2022-04-06T10:30:46Z

I agree that we should think of better organisation of composites, but that they should be in another PR. Merging this one now.

gerritholl added 6 commits March 22, 2022 10:54

Refactor areadefinition in VIIRS unit tests

35e910f

Refactor common code into fixtures

5220ddb

For the VIIRS composites unit tests, refactor common code into fixtures.

Add unit test for snow age RGB

b45a8d4

Add a unit test for the snow age RGB. This unit test currently fails, because it confirms, among other things, that there there is no units attribute present. Also add a reference to the snow age publication.

Add "no units" test for RatioSharpenedRGB

8a9cd23

Add a test to confirm that RGBs produced by the RatioSharpenedRGB have no units attribute.

Clear units attribute for SnowAge and RatioSharpened compositors

cbbe842

In the SnowAge and RatioSharpened compositors, clear the units attribute after the RGB has been produced.

Document removal of units from RGBs

6972258

gerritholl marked this pull request as ready for review March 22, 2022 10:33

gerritholl requested review from djhoese and mraspaud as code owners March 22, 2022 10:33

djhoese mentioned this pull request Mar 22, 2022

RGBs should never have units, but some do #2066

Closed

Merge branch 'main' into no-units-for-rgbs

dbeb30a

Merge branch 'main' into no-units-for-rgbs

72f3acb

mraspaud approved these changes Apr 6, 2022

View reviewed changes

mraspaud assigned gerritholl Apr 6, 2022

mraspaud added the enhancement code enhancements, features, improvements label Apr 6, 2022

mraspaud added the component:compositors label Apr 6, 2022

mraspaud merged commit 9d57970 into pytroll:main Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure RGBs do not have units attributes. #2068

Make sure RGBs do not have units attributes. #2068

gerritholl commented Mar 22, 2022 •

edited

gerritholl commented Mar 22, 2022

djhoese commented Mar 22, 2022

gerritholl commented Mar 22, 2022 •

edited

pnuu commented Mar 22, 2022 •

edited

djhoese commented Mar 22, 2022

gerritholl commented Mar 22, 2022

gerritholl commented Mar 22, 2022

djhoese commented Mar 22, 2022

gerritholl commented Mar 23, 2022

djhoese commented Mar 30, 2022

gerritholl commented Mar 31, 2022

djhoese commented Mar 31, 2022

codecov bot commented Apr 6, 2022 •

edited

coveralls commented Apr 6, 2022

mraspaud left a comment

mraspaud commented Apr 6, 2022

Make sure RGBs do not have units attributes. #2068

Make sure RGBs do not have units attributes. #2068

Conversation

gerritholl commented Mar 22, 2022 • edited

gerritholl commented Mar 22, 2022

djhoese commented Mar 22, 2022

gerritholl commented Mar 22, 2022 • edited

pnuu commented Mar 22, 2022 • edited

djhoese commented Mar 22, 2022

gerritholl commented Mar 22, 2022

gerritholl commented Mar 22, 2022

djhoese commented Mar 22, 2022

gerritholl commented Mar 23, 2022

djhoese commented Mar 30, 2022

gerritholl commented Mar 31, 2022

djhoese commented Mar 31, 2022

codecov bot commented Apr 6, 2022 • edited

Codecov Report

coveralls commented Apr 6, 2022

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud commented Apr 6, 2022

gerritholl commented Mar 22, 2022 •

edited

gerritholl commented Mar 22, 2022 •

edited

pnuu commented Mar 22, 2022 •

edited

codecov bot commented Apr 6, 2022 •

edited