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 fsspec support to li_l2_nc
reader
#2753
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2753 +/- ##
=======================================
Coverage 95.92% 95.93%
=======================================
Files 375 375
Lines 53167 53237 +70
=======================================
+ Hits 51003 51075 +72
+ Misses 2164 2162 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
satpy/readers/netcdf_utils.py
Outdated
try: | ||
attrs = v.attrs | ||
except AttributeError: | ||
attrs = v.__dict__ |
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'm not very familiar with this, but it feels like it deserves a test...
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 agree, but I have no idea how to test. We don't access any real files in the tests.
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.
Maybe we then should add one test with a real file?
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 have an idea, lets see if I can complete it today.
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.
f1bc699 should cover the addition. Fails with the original and passes with the current implementation for h5netcdf
backend (remote reading).
satpy/readers/netcdf_utils.py
Outdated
try: | ||
arr = xr.DataArray( | ||
variable[:], dims=variable.dimensions, attrs=attrs, name=variable.name) | ||
except ValueError: |
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.
Just curious, does the ValueError
come from the variable[:]
? Would it make more sense to have a try/except
dedicated to just the array creation and then a single xr.DataArray(...)
creation at the end (that uses the attrs
and var_arr
or whatever created from the previous try/excepts).
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, that's it. I refactored as suggested and added a test for the scalar case in 12854de
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 PR adds remote reading support to
li_l2_nc
reader via fsspec.The change in
NetCDF4FileHandler.collect_cache_vars()
for getting the attributes is that forh5netcdf
backend thev.__dict__
returns (forlongitude
dataset as an example):instead of
v.attrs
This could be wrapped in
dict(v.attrs)
but apparently this already work as dictionary so there's no need.The change is needed for the coordinates to be handled properly by setting
standard_name
, scale and offset.AUTHORS.md
if not there already