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 a reader for MODIS Level 3 files in CMG format. #2631

Merged
merged 32 commits into from
Nov 16, 2023

Conversation

simonrp84
Copy link
Member

@simonrp84 simonrp84 commented Nov 8, 2023

This PR adds a reader for MODIS Level 3 (daily / monthly / etc) products on the climate monitoring grid.

I'd like feedback on the change I made to hdfeos_base.py. The CMG data has metadata containing URLs with = signs, which confuses the _split_line method. My change means that only the first = is used to split strings, with the subsequent ones assumed as part of the metadata.
Personally, I don't think the try...except is needed as there should only ever be one important = in each string. But I wrote it this way in case there's some problem I've not thought of. Opinions welcome.

  • Tests added
  • Fully documented

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (6fb1d1e) 95.20% compared to head (0a18d2d) 95.22%.
Report is 12 commits behind head on main.

Files Patch % Lines
satpy/readers/modis_l3.py 96.36% 2 Missing ⚠️
.../tests/reader_tests/modis_tests/_modis_fixtures.py 97.91% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2631      +/-   ##
==========================================
+ Coverage   95.20%   95.22%   +0.01%     
==========================================
  Files         354      356       +2     
  Lines       51370    51539     +169     
==========================================
+ Hits        48908    49076     +168     
- Misses       2462     2463       +1     
Flag Coverage Δ
behaviourtests 4.22% <0.00%> (-0.02%) ⬇️
unittests 95.84% <98.17%> (+0.01%) ⬆️

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 Nov 8, 2023

Pull Request Test Coverage Report for Build 6892910354

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 161 of 164 (98.17%) changed or added relevant lines in 6 files are covered.
  • 8 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.01%) to 95.795%

Changes Missing Coverage Covered Lines Changed/Added Lines %
satpy/tests/reader_tests/modis_tests/_modis_fixtures.py 47 48 97.92%
satpy/readers/modis_l3.py 53 55 96.36%
Files with Coverage Reduction New Missed Lines %
satpy/tests/modifier_tests/test_angles.py 1 99.5%
satpy/tests/reader_tests/modis_tests/_modis_fixtures.py 1 98.23%
satpy/modifiers/angles.py 2 99.22%
satpy/readers/nwcsaf_nc.py 4 82.21%
Totals Coverage Status
Change from base Build 6847297703: 0.01%
Covered Lines: 49202
Relevant Lines: 51362

💛 - Coveralls

@simonrp84
Copy link
Member Author

Codefactor is moaning about the complexity of create_hdfeos_test_file and I'm not really sure what I'm supposed to do about it...all the if statements there are needed and it seems kinda wasteful to invent a whole new function just to avoid if..then.else. Suggestions appreciated!

satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
This is fixed, but not defined in the file. So we must
generate it ourselves with some assumptions.
"""
proj_param = "EPSG:4326"
Copy link
Member

Choose a reason for hiding this comment

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

Ok so this is just assuming a generic lon/lat coordinate system?

https://gdal.org/user/coordinate_epoch.html

Looks like a related discussion:

opengeospatial/geoparquet#52

Copy link
Member

Choose a reason for hiding this comment

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

I think your suggestion on slack to use OGC:CRS84 is good as a minimum. I could have sworn there was a CRS that was at least rooted in an exact date/time, but otherwise was similar to EPSG 4326. At least CRS84 has the same axis order that we use/expect.

satpy/tests/reader_tests/modis_tests/_modis_fixtures.py Outdated Show resolved Hide resolved
satpy/tests/reader_tests/modis_tests/test_modis_l3.py Outdated Show resolved Hide resolved
satpy/tests/reader_tests/modis_tests/_modis_fixtures.py Outdated Show resolved Hide resolved
@djhoese
Copy link
Member

djhoese commented Nov 8, 2023

CodeScene is happy now!

@djhoese djhoese added enhancement code enhancements, features, improvements component:readers labels Nov 13, 2023
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Nice job putting this together. I put a couple questions, some concerns, and other comments inline.

Otherwise, I'd like to officially state that I'd like something better than the -999/-9999 resolution stuff in the tests to describe these files. It just seems so hacky. If it must be this way then fine, but it feels like we can come up with something better.

satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/tests/reader_tests/modis_tests/test_modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
satpy/readers/modis_l3.py Outdated Show resolved Hide resolved
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

This looks good to me. I wouldn't mind a discussion on the usage of EPSG:4326 and I'd still like @mraspaud to look at the change to hdfeos_base.py, but otherwise this looks pretty good. Nice job sticking with it @simonrp84!

@simonrp84
Copy link
Member Author

Looking through the MODIS docs it seems like EPSG:4326 is what's used by the data provider, so I suggest that we stick with that in this PR.

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.

Looks good, but I would use the maxsplit everytime.

satpy/readers/viirs_edr.py Show resolved Hide resolved
@simonrp84
Copy link
Member Author

Looks good, but I would use the maxsplit everytime.

Sorted this, so always using maxsplit=1 now.

@djhoese
Copy link
Member

djhoese commented Nov 16, 2023

I'll see if I can do a quick check on the viirs edr reader so complete the tests for these changes. Then we can merge it.

@djhoese djhoese merged commit c09c147 into pytroll:main Nov 16, 2023
19 checks passed
@simonrp84 simonrp84 deleted the modis_l3 branch November 16, 2023 17:04
@simonrp84
Copy link
Member Author

That was a monster of a PR, turned out to be more complex than I was anticipating! Thanks for the help and comments along the way, really nice to get this merged.

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.

4 participants