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

Added initial version of an MSG BUFR reader and TOZ product yaml file #769

Merged
merged 34 commits into from
Nov 4, 2019

Conversation

ColinDuff
Copy link
Contributor

I have added a BUFR reader , currently for MSG BUFR products.

No test case for now as its still in the initial stages of development

@ColinDuff
Copy link
Contributor Author

Please note this reader requires the installation of the python-eccodes package

@mraspaud mraspaud added component:readers enhancement code enhancements, features, improvements labels May 16, 2019
@codecov
Copy link

codecov bot commented May 17, 2019

Codecov Report

Merging #769 into master will increase coverage by 0.02%.
The diff coverage is 91.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #769      +/-   ##
==========================================
+ Coverage   86.61%   86.63%   +0.02%     
==========================================
  Files         175      177       +2     
  Lines       26822    26948     +126     
==========================================
+ Hits        23231    23346     +115     
- Misses       3591     3602      +11
Impacted Files Coverage Δ
satpy/tests/reader_tests/__init__.py 98.27% <100%> (+0.03%) ⬆️
satpy/readers/seviri_l2_bufr.py 91.07% <91.07%> (ø)
satpy/tests/reader_tests/test_seviri_l2_bufr.py 92.59% <92.59%> (ø)
satpy/tests/writer_tests/test_cf.py 98.37% <0%> (-0.2%) ⬇️
satpy/writers/cf_writer.py 91.24% <0%> (-0.2%) ⬇️

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 283ca90...35b7441. Read the comment docs.

@coveralls
Copy link

coveralls commented May 17, 2019

Coverage Status

Coverage increased (+0.03%) to 86.634% when pulling 35b7441 on ColinDuff:msg_bufr_reader into 283ca90 on pytroll:master.

@mraspaud mraspaud added the PCW Pytroll Contributors' Week label May 21, 2019
@mraspaud
Copy link
Member

@ColinDuff could you add some tests to this ?

@ColinDuff
Copy link
Contributor Author

@mraspaud , hi , been busy with a lot of MPEF MSG work but now have the time to look at testing this change - hope to have it completed soon

@mraspaud
Copy link
Member

Sounds great, no rush :)

@ColinDuff
Copy link
Contributor Author

i have added some more seviri L2 Bufr products

@mraspaud mraspaud requested a review from sjoro October 16, 2019 07:33
Copy link
Collaborator

@sjoro sjoro left a comment

Choose a reason for hiding this comment

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

More yaml-file clean-up needed. Wrong reference documents given for seviri_l2_bufr-reader.
Also, the standard names, now mostly having count in YAML-files need to be checked against:
http://cfconventions.org/Data/cf-standard-names/68/build/cf-standard-name-table.html

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 putting the effort of reading bufr files, it's no small feat!

Here are my first comments/questions on top of the inline review:

  1. Do we really need that many readers ? One should suffice...
  2. You should add the reader(s) to the reader table in doc/index.rst
  3. Please run pre-commit on the PR and fix the styling issues

@ColinDuff
Copy link
Contributor Author

based on the review comments , in particular the opening of the file twice i realised the get_attributes function was working as i had hoped. As it was only used to determine the segment size a dictionary can be used instead.

Copy link
Collaborator

@sjoro sjoro left a comment

Choose a reason for hiding this comment

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

resolution for the the products is in my opinion wrong, all of the products are segmented, resolution should reflect the segment size.

Copy link
Collaborator

@sjoro sjoro 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 to me!

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. Great job on adding all these products in one got !
Maybe the next step is a PR on pytroll-examples to show the users how to use this ?

Copy link
Collaborator

@sjoro sjoro left a comment

Choose a reason for hiding this comment

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

seg_area_dict is not needed in seviri_l2_bufr.py, please remove. otherwise LGTM.

Copy link
Collaborator

@sjoro sjoro left a comment

Choose a reason for hiding this comment

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

LGTM

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 e958d7d into pytroll:master Nov 4, 2019
@ColinDuff ColinDuff deleted the msg_bufr_reader branch November 11, 2020 08:17
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 PCW Pytroll Contributors' Week
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants