Implement setncattr_string for attributes that are lists of strings #2045
Conversation
xarray/backends/netCDF4_.py
Outdated
|
|
||
| def is_list_of_strings(self, value): | ||
| if (np.asarray(value).dtype.kind in ['U', 'S'] and | ||
| np.asarray(value).size > 1): |
There was a problem hiding this comment.
E129 visually indented line with same indent as next logical line
|
OK, after some false starts, I think this is working, but the CI build is failing on CONDA_ENV=py35 and py36... but it's failing on a different test (specific to netcdf3, complaining about a temp file) and I can't see how it is impacted by my changes... thoughts? |
xarray/backends/netCDF4_.py
Outdated
| self.ds.setncattr(key, value) | ||
| if self.is_list_of_strings(value) and (self.format == 'NETCDF4'): | ||
| # encode as NC_STRING if attr is list of strings and NETCDF4 | ||
| self.ds.setncattr_string(key, value) |
There was a problem hiding this comment.
Can we adjust this to raise a sensible error message if the user has an old version of netCDF4-python installed which doesn't have this method?
xarray/backends/netCDF4_.py
Outdated
| if self.format != 'NETCDF4': | ||
| value = encode_nc3_attr_value(value) | ||
| self.ds.setncattr(key, value) | ||
| if self.is_list_of_strings(value) and (self.format == 'NETCDF4'): |
There was a problem hiding this comment.
We probably want to use this in all cases, even if a different format from NETCDF4 is selected. It's better to error than to improperly serialize data.
There was a problem hiding this comment.
OK, I tested with netCDF4 1.2.2 (last release before setncattr_string was introduced) and successfully catch the error with this latest commit. Is this along the lines of what you were thinking?
xarray/backends/netCDF4_.py
Outdated
| 'upgrade to netCDF4-python 1.2.4 or greater.') | ||
| raise AttributeError(msg) | ||
| else: | ||
| nc4_var.setncattr(k, v) |
There was a problem hiding this comment.
Could you consolidate this logic into a helper function (e.g., _set_attribute()) rather than repeating it twice?
|
I only have a vague grasp of proper object oriented programming practices; not sure if the helper functions should be within the class or what, but it does seem to work this way. |
shoyer
left a comment
shoyer
left a comment
There was a problem hiding this comment.
please merge in the latest changes from master, then we can easily verify that all tests pass with continuous integration.
xarray/tests/test_backends.py
Outdated
| repr(on_disk) | ||
| assert not on_disk['var1']._in_memory | ||
|
|
||
| def test_setncattr_string(self): |
There was a problem hiding this comment.
You might need to add this test specifically for the class with netCDF4 tests, making use of the self.roundtrip() method. Otherwise this is also going to run for cases where the list of strings attribute cannot be roundtripped (e.g., writing a netCDF3 file).
xarray/tests/test_backends.py
Outdated
| """ | ||
| Test to ensure setncattr_string is called on lists of strings | ||
| with NetCDF4 | ||
| """ |
There was a problem hiding this comment.
Nit: we generally avoid writing docstrings for test method unless they add substantial information beyond what's already obvious from the class/method name.
dnowacki-usgs
force-pushed
the
nc_string
branch
3 times, most recently
from
92110dc to
92224e0
Compare
|
@shoyer Thank you for the helpful feedback. I think I addressed all of your comments, and also expanded the tests. |
shoyer
left a comment
shoyer
left a comment
There was a problem hiding this comment.
Looks good to me, after one small doc change
doc/whats-new.rst
Outdated
| Enhancements | ||
| ~~~~~~~~~~~~ | ||
|
|
||
| - Support reading and writing lists of strings as attributes to netCDF |
There was a problem hiding this comment.
Minor nit: this should be "Support writing lists" only. We can already read lists of strings.
dnowacki-usgs
force-pushed
the
nc_string
branch
from
92224e0 to
94e2934
Compare
|
thanks! |
3 participants
whats-new.rstfor all changes andapi.rstfor new API (remove if this change should not be visible to users, e.g., if it is an internal clean-up, or if this is part of a larger project that will be documented later)