-
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
Sort reader table by name + diverse fixes #2745
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2745 +/- ##
==========================================
+ Coverage 95.89% 95.91% +0.02%
==========================================
Files 371 373 +2
Lines 52825 52967 +142
==========================================
+ Hits 50656 50803 +147
+ Misses 2169 2164 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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, thanks for the quick work!
Hi, Could you please set the status of my readers to |
@simonrp84 will do. Thanks for the feedback. |
satpy/etc/readers/agri_fy4b_l1.yaml
Outdated
description: FY-4B AGRI instrument HDF5 reader | ||
status: Beta | ||
supports_fsspec: 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.
@simonrp84 @BENR0 I could be wrong, but I think any reader that is based on the HDF5 utility file handler now supports fsspec because it uses open_file_or_filename
:
satpy/satpy/readers/hdf5_utils.py
Lines 46 to 48 in 754e706
try: | |
f_obj = open_file_or_filename(self.filename) | |
file_handle = h5py.File(f_obj, "r") |
No idea if it performs very well or if h5py works well with it, but I think the support is there.
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.
Good point. I think this is true. I have set this to true for those readers using "hdf5_utils" but have not tested this.
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.
Is this something that should be tested via unittests?
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 recently added tests for the helper function:
satpy/satpy/tests/test_readers.py
Lines 1207 to 1231 in e74729e
@pytest.mark.parametrize( | |
("file_thing", "create_read_func"), | |
[ | |
(lazy_fixture("local_netcdf_filename"), _open_xarray_default), | |
(lazy_fixture("local_netcdf_filename"), _open_xarray_netcdf4), | |
(lazy_fixture("local_netcdf_filename"), _open_xarray_h5netcdf), | |
(lazy_fixture("local_netcdf_path"), _open_xarray_default), | |
(lazy_fixture("local_netcdf_path"), _open_xarray_netcdf4), | |
(lazy_fixture("local_netcdf_path"), _open_xarray_h5netcdf), | |
(lazy_fixture("local_netcdf_fsspec"), _open_xarray_default), | |
(lazy_fixture("local_netcdf_fsspec"), _open_xarray_h5netcdf), | |
(lazy_fixture("local_netcdf_fsfile"), _open_xarray_default), | |
(lazy_fixture("local_netcdf_fsfile"), _open_xarray_h5netcdf), | |
(lazy_fixture("local_hdf5_filename"), _open_h5py), | |
(lazy_fixture("local_hdf5_path"), _open_h5py), | |
(lazy_fixture("local_hdf5_fsspec"), _open_h5py), | |
], | |
) | |
def test_open_file_or_filename(file_thing, create_read_func): | |
"""Test various combinations of file-like things and opening them with various libraries.""" | |
from satpy.readers import open_file_or_filename | |
read_func = create_read_func() | |
open_thing = open_file_or_filename(file_thing) | |
read_func(open_thing) |
Refactor automatic reader table to be sorted by name by default follwing @djhoese wish (on Slack).
Other changes: