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

Mcd12q1 draft #2905

Merged
merged 16 commits into from
Sep 20, 2024
Merged

Mcd12q1 draft #2905

merged 16 commits into from
Sep 20, 2024

Conversation

pdebuyl
Copy link
Contributor

@pdebuyl pdebuyl commented Sep 18, 2024

Add a reader for MODIS L3 data on the 500 sinusoidal grid. Data supported now is MCD12Q1.

  • Tests added
  • Fully documented

The reader has a test using the create_hdfeos_test_file fixture as other MODIS tests do.

For information, I used this to create a resampled land cover type at a coarser resolution. I can provide an example program if there is interest. The MODIS grid is described here: https://modis-land.gsfc.nasa.gov/MODLAND_grid.html

@pdebuyl
Copy link
Contributor Author

pdebuyl commented Sep 18, 2024

PS: in principle, I could add it to the MODIS L3 reader but it supported only the CMG grid so far, so I don't know if it is better to extend support in the existing modis_l3.py file or more simply by adding the class MCD12Q1HDFFileHandler that I added as an additional class in the MODIS L3 reader file. The yaml could point to the different classes for the different file types.

@coveralls
Copy link

coveralls commented Sep 18, 2024

Pull Request Test Coverage Report for Build 10940136250

Details

  • 99 of 108 (91.67%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 96.159%

Changes Missing Coverage Covered Lines Changed/Added Lines %
satpy/readers/hdfeos_base.py 5 6 83.33%
satpy/readers/mcd12q1.py 43 51 84.31%
Totals Coverage Status
Change from base Build 10884783684: -0.01%
Covered Lines: 52555
Relevant Lines: 54654

💛 - Coveralls

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 overall, I see the CI failing though :)
And just an nitpick comment inline

Comment on lines 186 to 189
if len(date) == 19:
return dt.datetime.strptime(date, "%Y-%m-%d %H:%M:%S")
else:
return dt.datetime.strptime(date, "%Y-%m-%d %H:%M:%S.%f")
Copy link
Member

Choose a reason for hiding this comment

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

This could be a function to avoid duplicating it a few lines below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :-)

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 91.50943% with 9 lines in your changes missing coverage. Please review.

Project coverage is 96.06%. Comparing base (5e27be4) to head (9e3b342).
Report is 133 commits behind head on main.

Files with missing lines Patch % Lines
satpy/readers/mcd12q1.py 84.00% 8 Missing ⚠️
satpy/readers/hdfeos_base.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2905    +/-   ##
========================================
  Coverage   96.05%   96.06%            
========================================
  Files         370      373     +3     
  Lines       54320    54465   +145     
========================================
+ Hits        52177    52321   +144     
- Misses       2143     2144     +1     
Flag Coverage Δ
behaviourtests 3.98% <0.00%> (-0.02%) ⬇️
unittests 96.15% <91.50%> (+<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.

@pdebuyl
Copy link
Contributor Author

pdebuyl commented Sep 19, 2024

Only a low score for codecov remaining. I find it hard to add another test for which the type of grid attribute would differ just for the sake of coverage. Is this ok?

I could remove the lines here: https://app.codecov.io/gh/pytroll/satpy/pull/2905/blob/satpy/readers/mcd12q1.py#L84 that don't seem necessary given that I put all of the variables in the YAML file.

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

@mraspaud mraspaud added enhancement code enhancements, features, improvements component:readers labels Sep 20, 2024
@mraspaud mraspaud merged commit 5d2b1fd into pytroll:main Sep 20, 2024
17 of 18 checks passed
@mraspaud
Copy link
Member

Coverage is good enough, thanks for adding this reader!

@pdebuyl pdebuyl deleted the mcd12q1_draft branch September 20, 2024 12:10
@pdebuyl
Copy link
Contributor Author

pdebuyl commented Sep 20, 2024

Thanks for reviewing! I prefer to have the code in the main branch of course :-)

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.

3 participants