-
Notifications
You must be signed in to change notification settings - Fork 303
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 SEVIRI level 2 AMV BUFR #2207
Conversation
satpy/readers/seviri_l2_bufr.py
Outdated
@@ -104,6 +105,10 @@ def __init__(self, filename, filename_info, filetype_info, with_area_definition= | |||
self.mpef_header['RectificationLongitude'] = f'E{int(rectification_longitude * 10):04d}' | |||
|
|||
self.with_adef = with_area_definition | |||
if filetype_info['file_type'] == 'seviri_l2_bufr_amv': | |||
logging.info("AMV BUFR data cannot be loaded with an area definition. Setting self.with_def = False.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably check if self.with_adef
isn't already False right (ex. if self.with_adef and filetype_info...:
)? Otherwise users will always get this no matter what they do.
Also, the .info
should maybe be a warning? Or maybe a regular warnings.warn
instead of a logging message? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi David,
Sorry for the late reply. The idea is to make sure that self.with_adef
is always set to False for the seviri_l2_bufr_amv
products, regardless of whether an area definition is (in this case wrongly) provided or not. But you are probably right in that checking if self.with_adef
is already False is probably clearer. I will see to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the logging.info
, I agree. I will change it to logging.warning
. Thanks for the suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to make sure that self.with_adef is always set to False for the seviri_l2_bufr_amv products
That's fine, but if I'm already following that desired behavior I shouldn't get extra information about how I'm not following the rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully agree. :)
Codecov Report
@@ Coverage Diff @@
## main #2207 +/- ##
==========================================
+ Coverage 94.16% 94.19% +0.03%
==========================================
Files 295 295
Lines 45361 45393 +32
==========================================
+ Hits 42714 42760 +46
+ Misses 2647 2633 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one small comment.
AUTHORS.md
Outdated
@@ -81,3 +81,4 @@ The following people have made contributions to this project: | |||
- [praerien (praerien)](https://github.com/praerien) | |||
- [Xin Zhang (zxdawn)](https://github.com/zxdawn) | |||
- [Yufei Zhu (yufeizhu600)](https://github.com/yufeizhu600) | |||
- [Manuel Carranza (manucarran)](https://github.com/manucarran) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put you name in alphabetical order in the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pull request has been created in order to add the missing SEVIRI level 2 AMV BUFR products to the corresponding satpy reader. The unit test file for the SEVIRI level 2 BUFR products has been updated in order to test the correct behaviour of the reader with AMV BUFR products (because these do not use the area definition).
As far as I see, there is no specific documentation to update for the SEVIRI level 2 BUFR reader.
flake8 satpy
AUTHORS.md
if not there already