Skip to content

Added driver parameter for h5netcdf#8360

Merged
dcherian merged 11 commits intopydata:mainfrom
zequihg50:main
Nov 15, 2023
Merged

Added driver parameter for h5netcdf#8360
dcherian merged 11 commits intopydata:mainfrom
zequihg50:main

Conversation

@zequihg50
Copy link
Contributor

@zequihg50 zequihg50 commented Oct 23, 2023

Following this change in h5netcdf, allow xarray to open HDF5 files using the ros3 VFD from HDF5.

@welcome
Copy link

welcome bot commented Oct 23, 2023

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

@dcherian
Copy link
Contributor

dcherian commented Oct 31, 2023

Thanks @zequihg50 . Is there a test file we can use or create to test with?

@dcherian dcherian requested a review from kmuehlbauer October 31, 2023 16:21
@kmuehlbauer
Copy link
Contributor

kmuehlbauer commented Oct 31, 2023

@zequihg50 This is looking good to me.

I've almost no experience with cloud access data, so please bear with me. It would be great if there is a permanent endpoint somewhere, or if it could be emulated within CI. Not only here, but also over at h5netcdf. Thanks!

@kmuehlbauer
Copy link
Contributor

kmuehlbauer commented Oct 31, 2023

The only thing we might have to think about is, that this will only take effect with the next h5netcdf release. For all other versions this will silently not take driver into account and use the current h5pyd approach.

@zequihg50
Copy link
Contributor Author

zequihg50 commented Nov 7, 2023

Now that the pull request for h5netcdf is merged, is there anything missing here? For testing I have suggested to use a file from netCDF examples.

@kmuehlbauer kmuehlbauer mentioned this pull request Nov 7, 2023
Closed
@kmuehlbauer
Copy link
Contributor

kmuehlbauer commented Nov 7, 2023

Now that the pull request for h5netcdf is merged, is there anything missing here? For testing I have suggested to use a file from netCDF examples.

Yes, a test would be nice indeed. Also, as mentioned over in #8423, it would be nice to distribute additional kwargs to the backend.

@kmuehlbauer kmuehlbauer mentioned this pull request Nov 8, 2023
Closed
4 tasks
kmuehlbauer
kmuehlbauer requested changes Nov 9, 2023
View reviewed changes
Copy link
Contributor

@kmuehlbauer kmuehlbauer left a comment

Choose a reason for hiding this comment

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

@zequihg50 I've added a couple of suggestions. Please let me know, if you can add the needed changes and a test.

Please also add an entry to whats-new.rst.

@zequihg50 zequihg50 mentioned this pull request Nov 10, 2023
Closed
1 task
kmuehlbauer
kmuehlbauer reviewed Nov 12, 2023
View reviewed changes
Copy link
Contributor

@kmuehlbauer kmuehlbauer left a comment

Choose a reason for hiding this comment

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

@zequihg50 I've added some code suggestion how to avoid some issues with CachingFileManager.

zequihg50 and others added 6 commits November 13, 2023 09:40
@zequihg50 @kmuehlbauer
Update xarray/backends/h5netcdf_.py
4ba3bb0 
Co-authored-by: Kai Mühlbauer <kmuehlbauer@wradlib.org>
@zequihg50 @kmuehlbauer
Update xarray/backends/h5netcdf_.py
6a1db94 
Co-authored-by: Kai Mühlbauer <kmuehlbauer@wradlib.org>
@zequihg50 @kmuehlbauer
Update xarray/backends/h5netcdf_.py
1f2a98e 
Co-authored-by: Kai Mühlbauer <kmuehlbauer@wradlib.org>
@zequihg50 @kmuehlbauer
Update xarray/backends/h5netcdf_.py
7a4978c 
Co-authored-by: Kai Mühlbauer <kmuehlbauer@wradlib.org>
@zequihg50
added test for h5netcdf ros3
d00b803
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
5e43a58 
for more information, see https://pre-commit.ci
@kmuehlbauer
Copy link
Contributor

kmuehlbauer commented Nov 13, 2023

@zequihg50 Thanks for adding the test. The test will only work for h5netcdf >= 1.3.0, though.

@dcherian Do we have to check for h5netcdf-version here? Or does it make sense to just let the test run if h5netcdf >= 1.3.0 is installed?

@dcherian
Copy link
Contributor

dcherian commented Nov 13, 2023

Or does it make sense to just let the test run if h5netcdf >= 1.3.0 is installed?

👍 @zequihg50 we use this type of decorator to run a test only if the version is greater than a min version:

xarray/xarray/tests/__init__.py

Lines 93 to 97 in 0c1ad54

# _importorskip does not work for development versions
has_pandas_version_two = Version(pd.__version__).major >= 2
requires_pandas_version_two = pytest.mark.skipif(
not has_pandas_version_two, reason="requires pandas 2.0.0"
)

@kmuehlbauer
Copy link
Contributor

kmuehlbauer commented Nov 15, 2023

LGTM! @zequihg50 Please merge xarray latest main into this PR branch and add an entry to https://github.com/pydata/xarray/blob/main/doc/whats-new.rst to make this enhancement visible.

@dcherian
Copy link
Contributor

dcherian commented Nov 15, 2023

Thanks @zequihg50 and @kmuehlbauer

@dcherian dcherian merged commit 1411474 into pydata:main Nov 15, 2023
@welcome
Copy link

welcome bot commented Nov 15, 2023

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again! celebration gif

@kmuehlbauer
Copy link
Contributor

kmuehlbauer commented Nov 15, 2023

Thanks @zequihg50 for pushing this through!

dcherian added a commit to rabernat/xarray that referenced this pull request Nov 29, 2023
@dcherian
Merge branch 'main' into fix-zarr-append
8cdce90 
* main:
  [skip-ci] dev whats-new (pydata#8467)
  2023.11.0 Whats-new (pydata#8461)
  migrate the other CI to python 3.11 (pydata#8416)
  preserve vlen string dtypes, allow vlen string fill_values (pydata#7869)
  Pin mypy < 1.7 (pydata#8458)
  Fix typos found by codespell (pydata#8457)
  [skip-ci] Small updates to IO docs. (pydata#8452)
  Deprecate certain cftime frequency strings following pandas (pydata#8415)
  Added driver parameter for h5netcdf (pydata#8360)
  Raise exception in to_dataset if resulting variable is also the name of a coordinate (pydata#8433)
  Automatic region detection and transpose for `to_zarr()` (pydata#8434)
  remove `cdms2` (pydata#8441)
  Remove PseudoNetCDF (pydata#8446)
  Pin pint to >=0.22 (pydata#8445)
  Remove keep_attrs from resample signature (pydata#8444)
  Rename `to_array` to `to_dataarray` (pydata#8438)
  Add missing DataArray.dt.total_seconds() method (pydata#8435)
  Declare Dataset, DataArray, Variable, GroupBy unhashable (pydata#8392)
@kmuehlbauer kmuehlbauer mentioned this pull request May 30, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kmuehlbauer kmuehlbauer Awaiting requested review from kmuehlbauer

Assignees

No one assigned

Labels

io topic-backends

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zequihg50 @dcherian @kmuehlbauer