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

Rename the fci_l1c_fdhsi reader to fci_l1c_nc #1712

Merged
merged 18 commits into from Jun 2, 2021

Conversation

ameraner
Copy link
Member

@ameraner ameraner commented Jun 1, 2021

This PR renames the fci_l1c_fdhsi reader to fci_l1c_nc, in order to have a single reader for all FCI L1c files (FDHSI and HRFI). Please refer to #1684 for the discussion.

The old reader name is put into the PENDING_OLD_READER_NAMES mechanism: users calling the old name will get a FutureWarning, and Satpy will then automatically use the new reader name. Shall be moved to OLD_READER_NAMES when sufficient notice time has passed.

@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #1712 (1cde40e) into main (217c6e2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1712      +/-   ##
==========================================
+ Coverage   92.43%   92.44%   +0.01%     
==========================================
  Files         258      258              
  Lines       38273    38273              
==========================================
+ Hits        35376    35382       +6     
+ Misses       2897     2891       -6     
Flag Coverage Δ
behaviourtests 4.84% <8.33%> (ø)
unittests 92.98% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
satpy/readers/__init__.py 97.14% <100.00%> (+1.22%) ⬆️
satpy/readers/fci_l1c_nc.py 97.97% <100.00%> (ø)
satpy/tests/reader_tests/test_fci_l1c_nc.py 100.00% <100.00%> (ø)
satpy/tests/test_readers.py 98.86% <0.00%> (+0.56%) ⬆️

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 217c6e2...1cde40e. Read the comment docs.

@ameraner ameraner self-assigned this Jun 1, 2021
@ameraner
Copy link
Member Author

ameraner commented Jun 1, 2021

@sjoro @gerritholl FYI

@coveralls
Copy link

coveralls commented Jun 1, 2021

Coverage Status

Coverage increased (+0.02%) to 92.935% when pulling 1cde40e on ameraner:feature_rename_fci_l1c_nc_reader into 217c6e2 on pytroll:main.

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.

Thanks for proposing this, I think it's a great idea to change the reader name to something more generic. I have two small suggestion, otherwise it looks great to me :)

@@ -30,7 +30,7 @@ satpy/etc/readers/avhrr_l1b_gaclac.yaml @mraspaud @sfinkens
satpy/etc/readers/avhrr_l1b_hrpt.yaml @mraspaud
satpy/etc/readers/clavrx.yaml @djhoese
satpy/etc/readers/electrol_hrit.yaml @sfinkens @mraspaud
satpy/etc/readers/fci_l1c_fdhsi.yaml @mraspaud
satpy/etc/readers/fci_l1c_nc.yaml @mraspaud
Copy link
Member

Choose a reason for hiding this comment

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

you and @gerritholl have worked more on this reader than me, maybe you should be the new code owners?

Suggested change
satpy/etc/readers/fci_l1c_nc.yaml @mraspaud
satpy/etc/readers/fci_l1c_nc.yaml @ameraner @gerritholl

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for offering! I can gladly be codeowner. @gerritholl do you agree as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, works for me. Now I know how GitHub figures out the code owners :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks!

@@ -68,7 +68,7 @@ satpy/readers/clavrx.py @djhoese
satpy/readers/electrol_hrit.py @sfinkens @mraspaud
satpy/readers/eps_l1b.py @mraspaud @pnuu @adybbroe
satpy/readers/eum_base.py @sjoro @sfinkens @adybbroe
satpy/readers/fci_l1c_fdhsi.py @mraspaud
satpy/readers/fci_l1c_nc.py @mraspaud
Copy link
Member

Choose a reason for hiding this comment

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

Same here:

Suggested change
satpy/readers/fci_l1c_nc.py @mraspaud
satpy/readers/fci_l1c_nc.py @ameraner @gerritholl

@djhoese
Copy link
Member

djhoese commented Jun 1, 2021

Do any example notebooks in pytroll-examples need to be updated? Can someone create an issue for if/when the old reader names should be completely deprecated/removed?

@ameraner
Copy link
Member Author

ameraner commented Jun 1, 2021

There is no example for FCI data in pytroll-examples (yet). I created a reminder-issue as proposed.

Copy link
Collaborator

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few minor points from my side.

description: >
Reader for FCI FDSHI data in NetCDF4 format.
Reader for FCI L1c data in NetCDF4 format.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be "1C" (lines 3 and 4) or "1c" (lines 6 and 8)? What is the standard spelling for the letter in levels? As it stands now, it's inconsistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The consistency of this spelling should be checked in the rest of the PR as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. It's hard to find a standard as the EUM documentation is also not consistent :) But L1c seems the preferred way. I have updated the PR to use L1c consistently.

@@ -161,7 +165,7 @@ class using the :mod:`~satpy.Scene.load` method with the reader

def __init__(self, filename, filename_info, filetype_info):
"""Initialize file handler."""
super(FCIFDHSIFileHandler, self).__init__(filename, filename_info,
super(FCIL1CNCFileHandler, self).__init__(filename, filename_info,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We support only Python 3 now, so this can be simplified to super().__init__(...).

Copy link
Member Author

Choose a reason for hiding this comment

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

True yes. Updated accordingly.

@ameraner
Copy link
Member Author

ameraner commented Jun 2, 2021

Thanks for all your inputs! I have updated the PR. I also took the occasion to slightly update the docs example to include the upper_right_corner and .values extraction.

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 merged commit 236e6d5 into pytroll:main Jun 2, 2021
@ameraner
Copy link
Member Author

ameraner commented Jun 2, 2021

Thank you all for your help and the quick review!

@ameraner ameraner deleted the feature_rename_fci_l1c_nc_reader branch May 17, 2022 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename fci_l1c_fdhsi to fci_l1c_nc
5 participants