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 FY-4A AGRI level 1 data #751

Merged
merged 43 commits into from Aug 8, 2019

Conversation

@zxdawn
Copy link
Contributor

commented May 6, 2019

This PR adds a reader for the FY-4A L1 data format which are HDF5 files.

  • Closes #603
  • Tests added
  • Tests passed
  • Passes git diff origin/master -- "*py" | flake8 --diff
  • Fully documented
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
zxdawn added 2 commits May 6, 2019
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved

@mraspaud mraspaud changed the title Add yaml and py file of FY-4A Add a reader for FY-4A AGRI level 1 data May 6, 2019

@mraspaud

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Also, don't forget to add your name to the AUTHORS.md file :)

@mraspaud
Copy link
Member

left a comment

Good work so far! Try to keep the data lazy as much as possible.

satpy/readers/agrl_l1.py Outdated Show resolved Hide resolved
@djhoese

This comment has been minimized.

Copy link
Member

commented May 7, 2019

I just remembered, could you also add an entry to the bottom of doc/source/index.rst like the other readers there?

@mraspaud
Copy link
Member

left a comment

Thank you so much for you comprehensive work, and very good job on the tests, the coverage of your reader is 100%!
I have commented some small things that can be improved slightly, mainly about the metadata to provide on the dataset. Regarding adding support for the GEO file, if you feel like doing it now, go ahead, but it's fine if you do another PR in the future.
I downloaded some test data, so I'll be testing this later.
Finally, please run flake8-docstring on your code, or pydocstyle to do the minor fixes on the docstrings.
Thanks again for a great contribution!

AUTHORS.md Show resolved Hide resolved
satpy/etc/readers/agri_l1.yaml Show resolved Hide resolved
satpy/readers/agri_l1.py Outdated Show resolved Hide resolved
satpy/readers/agri_l1.py Show resolved Hide resolved
satpy/tests/reader_tests/test_agri_l1.py Outdated Show resolved Hide resolved
zxdawn and others added 10 commits Jun 27, 2019
1. add more attrs of satellite;
2. use value provided in the HDF file;
3. add sweep to proj_dict
@mraspaud

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

agri_overview
agri_true_color

@mraspaud

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

I'm happy with this. @djhoese any final comments before we assign this to a milestone (ie ready to merge) ?

@djhoese
Copy link
Member

left a comment

Nice work. I had a couple questions about things that may be possible to clean up. Otherwise I'd like you to update the license headers to match other modules in Satpy.

satpy/readers/agri_l1.py Outdated Show resolved Hide resolved
satpy/readers/agri_l1.py Outdated Show resolved Hide resolved
satpy/readers/agri_l1.py Outdated Show resolved Hide resolved
satpy/readers/agri_l1.py Show resolved Hide resolved
satpy/readers/agri_l1.py Outdated Show resolved Hide resolved
satpy/tests/reader_tests/test_agri_l1.py Outdated Show resolved Hide resolved
@sfinkens
Copy link
Collaborator

left a comment

Nice work, thank you very much! Just nitpicking...

satpy/etc/readers/agri_l1.yaml Outdated Show resolved Hide resolved
satpy/readers/agri_l1.py Outdated Show resolved Hide resolved
zxdawn added 2 commits Jun 28, 2019

@mraspaud mraspaud added this to the v0.17.0 milestone Jun 28, 2019

@mraspaud mraspaud merged commit 314c314 into pytroll:master Aug 8, 2019

5 checks passed

CodeFactor No issues found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 84.206%
Details
stickler-ci No lint errors found
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.