MNT: towards new h5netcdf/netcdf4 features #9509
Conversation
kmuehlbauer
force-pushed
the
h5netcdf-new-features
branch
from
f592de3
to
7845e81
Compare
| .. ipython:: python | ||
| :okwarning: | ||
|
|
||
| # Writing complex valued data |
There was a problem hiding this comment.
Maybe we should keep the example and highlight the new auto_complex=True of netcdf4-python?
There was a problem hiding this comment.
I think it would be very useful to have an example.
Is it still true that a warning is raised? Is that warning xarray or h5netcdf?
There was a problem hiding this comment.
There will be no warning raised if capable versions are detected.
| @@ -566,29 +566,12 @@ This is not CF-compliant but again facilitates roundtripping of xarray datasets. | |||
| Invalid netCDF files | |||
| ~~~~~~~~~~~~~~~~~~~~ | |||
|
|
|||
| The library ``h5netcdf`` allows writing some dtypes (booleans, complex, ...) that aren't | |||
There was a problem hiding this comment.
It boils down, that we are approaching feature equality between netcdf4-python and h5netcdf. It seems, that only reference objects can't be handled by netcdf-c.
There was a problem hiding this comment.
Todo: check boolean enums
| @@ -537,6 +537,7 @@ def _choose_float_dtype( | |||
| if dtype.itemsize <= 2 and np.issubdtype(dtype, np.integer): | |||
| return np.float32 | |||
| # For all other types and circumstances, we just use float64. | |||
| # Todo: with nc-complex from netcdf4-python >= 1.7.0 this is available | |||
There was a problem hiding this comment.
Note to self: Add a test for this and fix it!
There was a problem hiding this comment.
Maybe leave it to dedicated follow-up PR?
There was a problem hiding this comment.
Will this be an issue? nc-complex still stores the data as floats, just either in a compound type or with an extra dimension (netCDF-3).
Although, HDF5 1.15 will have native support for complex numbers, so that may be a future consideration (I suspect it will still be some time before netCDF itself gets native support)
There was a problem hiding this comment.
Yes, maybe it will work out of the box.
for more information, see https://pre-commit.ci
kmuehlbauer
force-pushed
the
h5netcdf-new-features
branch
from
801216c
to
b78c42e
Compare
| .. ipython:: python | ||
| :okwarning: | ||
|
|
||
| # Writing complex valued data |
There was a problem hiding this comment.
I think it would be very useful to have an example.
Is it still true that a warning is raised? Is that warning xarray or h5netcdf?
| @@ -1230,6 +1231,7 @@ def to_netcdf( | |||
| compute: bool = True, | |||
| multifile: Literal[False] = False, | |||
| invalid_netcdf: bool = False, | |||
| auto_complex: bool | None = None, | |||
There was a problem hiding this comment.
What does auto_complex = None mean? Is it the same as auto_complex = False?
EDIT: Ah, to avoid passing the argument to h5netcdf? But invalid_netcdf is just a plain bool, so is that handled differently for netCDF4?
Should we also consider defaulting to True? I'm definitely interested in finding out if there are any cases where this results in unwanted behaviour
There was a problem hiding this comment.
Yeah, not sure, if I handled that correctly. It's more or less a security measure against feeding the kwarg to versions which can't handle those yet. I think this was the simplest solution without adding a whole bunch of boilerplate code. But maybe there is an easier solution to that.
| if auto_complex is not None: | ||
| kwargs["auto_complex"] = auto_complex |
There was a problem hiding this comment.
Is this to avoid passing auto_complex to the h5netcdf engine?
There was a problem hiding this comment.
To not feed it to old versions which can't digest it.
| @@ -537,6 +537,7 @@ def _choose_float_dtype( | |||
| if dtype.itemsize <= 2 and np.issubdtype(dtype, np.integer): | |||
| return np.float32 | |||
| # For all other types and circumstances, we just use float64. | |||
| # Todo: with nc-complex from netcdf4-python >= 1.7.0 this is available | |||
There was a problem hiding this comment.
Will this be an issue? nc-complex still stores the data as floats, just either in a compound type or with an extra dimension (netCDF-3).
Although, HDF5 1.15 will have native support for complex numbers, so that may be a future consideration (I suspect it will still be some time before netCDF itself gets native support)
Co-authored-by: Peter Hill <zed.three@gmail.com>
|
Thanks @ZedThree for taking the time to review. I've added couple of comments. |
| if auto_complex is not None: | ||
| kwargs["auto_complex"] = auto_complex | ||
|
|
There was a problem hiding this comment.
Continuing the discussion above about making auto_complex: bool, then this could be:
| if auto_complex is not None: | |
| kwargs["auto_complex"] = auto_complex | |
| if auto_complex: | |
| if engine != "netcdf4": | |
| raise ValueError( | |
| f"unrecognised option 'auto_complex' for engine {engine}" | |
| ) | |
| kwargs["auto_complex"] = auto_complex | |
This would mean we couldn't default to having this on though.
EDIT: Ah, but this also needs to handle earlier versions without the argument too. Hmmm, trickier!
whats-new.rstapi.rst