Skip to content
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 support for Sentinel-2 MSI L2A SAFE datasets #2783

Merged
merged 54 commits into from
Jun 7, 2024
Merged

Add support for Sentinel-2 MSI L2A SAFE datasets #2783

merged 54 commits into from
Jun 7, 2024

Conversation

yukaribbba
Copy link
Contributor

@yukaribbba yukaribbba commented Apr 18, 2024

Add support for Sentinel-2 MSI L2A SAFE datasets

  • Tests added
  • Fully documented

Composites family:
image
image
image

Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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 Coverage Δ
behaviourtests 4.04% <0.00%> (-0.05%) ⬇️
unittests 96.01% <100.00%> (-0.04%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coveralls
Copy link

coveralls commented Apr 18, 2024

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.

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%
Totals Coverage Status
Change from base Build 9157044280: 0.008%
Covered Lines: 51561
Relevant Lines: 53687

💛 - Coveralls

@simonrp84
Copy link
Member

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

@yukaribbba
Copy link
Contributor Author

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

🍺

@yukaribbba
Copy link
Contributor Author

pre-commit.ci autofix

@yukaribbba
Copy link
Contributor Author

pre-commit.ci autofix

@mraspaud
Copy link
Member

Yes

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice that there is so little to change the msi reader python code! I have some questions inline.

satpy/dataset/dataid.py Outdated Show resolved Hide resolved
satpy/etc/composites/msi.yaml Outdated Show resolved Hide resolved
satpy/etc/composites/msi.yaml Outdated Show resolved Hide resolved
satpy/etc/composites/msi.yaml Outdated Show resolved Hide resolved
satpy/etc/readers/msi_safe_l2a.yaml Outdated Show resolved Hide resolved
@yukaribbba
Copy link
Contributor Author

pre-commit.ci autofix

@yukaribbba yukaribbba requested a review from mraspaud May 28, 2024 03:18
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question really, why do the band names need to be different in l1 and l2?

satpy/etc/readers/msi_safe_l2a.yaml Outdated Show resolved Hide resolved
@yukaribbba
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@djhoese What do you think about this?

@djhoese djhoese added enhancement code enhancements, features, improvements component:readers labels Jun 4, 2024
@djhoese
Copy link
Member

djhoese commented Jun 4, 2024

@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
Copy link
Member

djhoese commented Jun 5, 2024

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

CMIP_C01: # Cloud Moisture Image Products Channel 1
name: C01
wavelength: [0.450, 0.470, 0.490]
calibration: reflectance
file_key: CMI
file_type: [abi_l2_cmip_c01, abi_l2_mcmip]

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
Copy link
Member

mraspaud commented Jun 5, 2024

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

CMIP_C01: # Cloud Moisture Image Products Channel 1
name: C01
wavelength: [0.450, 0.470, 0.490]
calibration: reflectance
file_key: CMI
file_type: [abi_l2_cmip_c01, abi_l2_mcmip]

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
Copy link
Contributor Author

k, we'll just drop it.

@simonrp84
Copy link
Member

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
Copy link
Contributor Author

@simonrp84 Thx for reminding! :)

@yukaribbba yukaribbba requested a review from mraspaud June 5, 2024 15:19
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot for putting all the efforts in improving the tests also!

@mraspaud mraspaud merged commit 367016e into pytroll:main Jun 7, 2024
17 of 19 checks passed
@yukaribbba yukaribbba deleted the add_support_msi_l2a branch June 8, 2024 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:readers enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants