-
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
Fix 'ahi_hsd' reader crashing when 'observation_timeline' was invalid #2107
Conversation
…meline was fill value.
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 fix! Just one comment regarding readability. Would you mind adding a test for this?
Codecov Report
@@ Coverage Diff @@
## main #2107 +/- ##
=======================================
Coverage 93.95% 93.96%
=======================================
Files 283 283
Lines 42881 42901 +20
=======================================
+ Hits 40290 40310 +20
Misses 2591 2591
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 a lot for adding the tests! Just one tiny comment :)
'satellite': 'Himawari-8', | ||
'observation_area': 'FLDK', | ||
'observation_start_time': 58413.12523839, | ||
'observation_end_time': 58413.12562439, | ||
'observation_timeline': '0300', |
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.
Does removing the np.array
reflect what the real code is getting? At least in the NetCDF/HDF5 readers these libraries have cases where they return a scalar array of a single string and we have to do extra work to make sure they are converted to plain strings correctly. If you kept the original np.array
usage and converted 300 to a string would that imitate the files well enough?
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.
Yes, it does: While debugging I had some print
statements inside the ahi_hsd.py
file and they showed that there's no numpy arrays for these values, they're all simple strings or floats.
Tests are failing but I think the failures are not to do with the changes here. |
The MacOS job seems to be a hiccup, the other one is the unstable environment which is failing because of changes in dask that are incompatible with xarray. We can track the dask/xarray issue at pydata/xarray#6578 I've restarted the MacOS job. |
Thanks, seems to have worked now |
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!
If an AHI data file contains fill value for the
'observation_timeline'
property then theahi_hsd
reader crashes when it tried to round the observation times across segments.This PR checks whether the timeline is valid and only performs rounding if it is. If invalid, no rounding is done and a warning is shown to the user.