Skip to content
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

towards new h5netcdf/netcdf4 features #9509

Merged
merged 21 commits into from
Oct 2, 2024

Conversation

kmuehlbauer
Copy link
Contributor

@kmuehlbauer kmuehlbauer commented Sep 17, 2024

doc/user-guide/io.rst Show resolved Hide resolved
@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Todo: check boolean enums

xarray/backends/h5netcdf_.py Outdated Show resolved Hide resolved
xarray/backends/netCDF4_.py Outdated Show resolved Hide resolved
xarray/coding/variables.py Show resolved Hide resolved
xarray/tests/test_backends.py Outdated Show resolved Hide resolved
@kmuehlbauer kmuehlbauer marked this pull request as ready for review September 18, 2024 08:21
@kmuehlbauer kmuehlbauer reopened this Sep 18, 2024
@kmuehlbauer kmuehlbauer added the run-upstream Run upstream CI label Sep 18, 2024
@kmuehlbauer
Copy link
Contributor Author

@ZedThree @bzah it would be great if you could have a look here. This brings nc-complex (netcdf4/h5netcdf) and enum handling (h5netcdf) to the backends. This is not released yet in h5netcdf, but in latest main-branch (checked with upstream CI run). If you have anything to add, please let me know.

Copy link
Contributor

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

doc/user-guide/io.rst Show resolved Hide resolved
@@ -1230,6 +1231,7 @@ def to_netcdf(
compute: bool = True,
multifile: Literal[False] = False,
invalid_netcdf: bool = False,
auto_complex: bool | None = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

xarray/backends/netCDF4_.py Show resolved Hide resolved
xarray/backends/netCDF4_.py Outdated Show resolved Hide resolved
xarray/backends/netCDF4_.py Show resolved Hide resolved
xarray/coding/variables.py Show resolved Hide resolved
Co-authored-by: Peter Hill <zed.three@gmail.com>
@kmuehlbauer
Copy link
Contributor Author

Thanks @ZedThree for taking the time to review. I've added couple of comments.

@kmuehlbauer
Copy link
Contributor Author

@pydata/xarray This seems to be ready now for review. This brings in adaptions for changes ongoing in h5netcdf/netcdf4-python wrt complex numbers and enums.

@kmuehlbauer kmuehlbauer changed the title MNT: towards new h5netcdf/netcdf4 features towards new h5netcdf/netcdf4 features Sep 23, 2024
Copy link
Contributor

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @kmuehlbauer!

I do think it would be clearer with having auto_complex just be bool (I don't think allowing it to be None gets us anything over setting it False by default), and with a check that when setting it we are using the netcdf4 engine, but that's a minor quibble.

@kmuehlbauer kmuehlbauer added the plan to merge Final call for comments label Sep 30, 2024
@kmuehlbauer
Copy link
Contributor Author

Upstream and upstream mypy failures are unrelated. I'm going to merge this the next day.

@kmuehlbauer kmuehlbauer merged commit c16fa1e into pydata:main Oct 2, 2024
27 of 29 checks passed
@kmuehlbauer kmuehlbauer deleted the h5netcdf-new-features branch October 2, 2024 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments run-upstream Run upstream CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Writing complex numbers to netCDF Add writing complex data to docs
2 participants