Add support for Sentinel-2 MSI L2A SAFE datasets

yukaribbba commented 5 months ago

[x] Tests added
[x] Fully documented

Composites family:

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.91%. Comparing base (3b9c04e) to head (ab55c4a). Report is 516 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #2783 +/- ## ========================================== - Coverage 95.95% 95.91% -0.04% ========================================== Files 379 366 -13 Lines 53888 53504 -384 ========================================== - Hits 51708 51321 -387 - Misses 2180 2183 +3 ``` | [Flag](https://app.codecov.io/gh/pytroll/satpy/pull/2783/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | Coverage Δ | | |---|---|---| | [behaviourtests](https://app.codecov.io/gh/pytroll/satpy/pull/2783/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `4.04% <0.00%> (-0.05%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/pytroll/satpy/pull/2783/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll) | `96.01% <100.00%> (-0.04%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pytroll#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 9386597470

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

94 of 94 (100.0%) changed or added relevant lines in 2 files are covered.
572 unchanged lines in 61 files lost coverage.
Overall coverage increased (+0.008%) to 96.04%

Files with Coverage Reduction	New Missed Lines	%
satpy/readers/glm_l2.py	1	98.55%
satpy/readers/ascat_l2_soilmoisture_bufr.py	1	97.44%
satpy/tests/multiscene_tests/test_save_animation.py	1	99.1%
satpy/tests/reader_tests/_li_test_utils.py	1	99.26%
satpy/readers/msi_safe.py	1	98.21%
satpy/dataset/metadata.py	1	99.26%
satpy/tests/reader_tests/test_li_l2_nc.py	1	99.76%
satpy/tests/reader_tests/test_ascat_l2_soilmoisture_bufr.py	1	97.59%
satpy/tests/reader_tests/test_mersi_l1b.py	1	99.71%
satpy/readers/mws_l1b.py	1	98.52%
<!--	Total:	572	-->

Totals
Change from base Build 9157044280:	0.008%
Covered Lines:	51561
Relevant Lines:	53687

💛 - Coveralls

simonrp84 commented 5 months ago

Thanks for adding this, it was on my to-do list but you got there first :-)

yukaribbba commented 5 months ago

Thanks for adding this, it was on my to-do list but you got there first :-)

:beer:

yukaribbba commented 5 months ago

pre-commit.ci autofix

yukaribbba commented 5 months ago

pre-commit.ci autofix

yukaribbba commented 5 months ago

pre-commit.ci autofix

yukaribbba commented 5 months ago

@simonrp84 If you got time pls also take a look. :D

mraspaud commented 5 months ago

Thanks a lot for this new reader functionality @yukaribbba ! One general comment before I dive deeper into this, could you make this a separate yaml file for this reader? The python code can stay with the l1 reader, but it will be easier to have a separate yaml file as this will make it effectively separate from the l1 reader for the user.

yukaribbba commented 5 months ago

Sure!

yukaribbba commented 5 months ago

The two YAMLs are separated now.

yukaribbba commented 4 months ago

@mraspaud Could you take some time to review this?

mraspaud commented 4 months ago

Yes

yukaribbba commented 4 months ago

pre-commit.ci autofix

yukaribbba commented 3 months ago

Well to be honest I'm not really sure about this but I want to make them look different so that users wouldn't mix them up. That's the first thing came into my mind...

yukaribbba commented 3 months ago

We have VIIRS SDR/EDR/L1B readers that can be considered as different process levels. L1B and SDR have the same band names while EDR has its own. So.......

yukaribbba commented 3 months ago

@djhoese What do you think about this?

djhoese commented 3 months ago

@yukaribbba I'm on parental leave so not sure I'll have time to review this. I'll try to take a look tonight, but have other work things I need to get to.

djhoese commented 3 months ago

Are you specifically asking me about the naming thing? Here is another example:

https://github.com/pytroll/satpy/blob/7a7740ab524a1ff61ad17e38c31b4f48e346b6fc/satpy/etc/readers/abi_l2_nc.yaml#L19-L24

ABI L2 files include the original L1b bands but calibrated to reflectances and radiances. With discussions like this in the past it has come down to: are users likely to load/use L1 and L2 files as the same time? The answer is typically no. If they have L2 files they're usually looking for the higher-level products (AOD, etc) not the band reflectances and radiances. For the VIIRS example, SDR and L1B are two different software packages from two different organizations producing essentially the same output so there is no need for the products/bands to be named something different. The EDRs are almost always higher level products only available from those higher level files (L2/EDR). Otherwise a good general rule is to give products the names used in the files or by the organization in charge of the spacecraft or algorithm.

I know nothing of these files or this instrument, but I think the suffix (L2A) should be dropped. My main reason is that I'm guessing users will rarely be loading from both readers at the same time. If this is an incorrect assumption then maybe they do need special names. Even then, only the bands should have special names, not the higher level products.

mraspaud commented 3 months ago

Are you specifically asking me about the naming thing? Here is another example:

https://github.com/pytroll/satpy/blob/7a7740ab524a1ff61ad17e38c31b4f48e346b6fc/satpy/etc/readers/abi_l2_nc.yaml#L19-L24

ABI L2 files include the original L1b bands but calibrated to reflectances and radiances. With discussions like this in the past it has come down to: are users likely to load/use L1 and L2 files as the same time? The answer is typically no. If they have L2 files they're usually looking for the higher-level products (AOD, etc) not the band reflectances and radiances. For the VIIRS example, SDR and L1B are two different software packages from two different organizations producing essentially the same output so there is no need for the products/bands to be named something different. The EDRs are almost always higher level products only available from those higher level files (L2/EDR). Otherwise a good general rule is to give products the names used in the files or by the organization in charge of the spacecraft or algorithm.

I know nothing of these files or this instrument, but I think the suffix (L2A) should be dropped. My main reason is that I'm guessing users will rarely be loading from both readers at the same time. If this is an incorrect assumption then maybe they do need special names. Even then, only the bands should have special names, not the higher level products.

Here is an example for OLCI, where level1 and level2 channel have the same name, but not same modifiers and standard name for example: https://github.com/pytroll/satpy/blob/main/satpy/etc/readers/olci_l1b.yaml#L61-L74 https://github.com/pytroll/satpy/blob/main/satpy/etc/readers/olci_l2.yaml#L96-L107

yukaribbba commented 3 months ago

k, we'll just drop it.

simonrp84 commented 3 months ago

The above discussion made me realise that the standard_names for the L2 reflectance datasets are wrong. At present, they are:

reflectance:
        standard_name: toa_bidirectional_reflectance
        units: "%"
      radiance:
        standard_name: toa_outgoing_radiance_per_unit_wavelength
        units: W m-2 um-1 sr-1

But as these are L2, they should instead be boa_... as atmospheric correction has been performed.

yukaribbba commented 3 months ago

@simonrp84 Thx for reminding! :)

pytroll / satpy