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.

Sign up for GitHub

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

Jump to bottom

Added driver parameter for h5netcdf #8360

Merged
merged 11 commits into from
Nov 15, 2023
Merged

Added driver parameter for h5netcdf #8360

merged 11 commits into from
Nov 15, 2023

Conversation

zequihg50
Copy link
Contributor

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

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

@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

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

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

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
Open
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.

xarray/backends/h5netcdf_.py Show resolved Hide resolved
xarray/backends/h5netcdf_.py Show resolved Hide resolved
xarray/backends/h5netcdf_.py Show resolved Hide resolved
xarray/backends/h5netcdf_.py Show resolved Hide resolved
@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.

xarray/backends/h5netcdf_.py Show resolved Hide resolved
xarray/backends/h5netcdf_.py Show resolved Hide resolved
xarray/backends/h5netcdf_.py Show resolved Hide resolved
xarray/backends/h5netcdf_.py Show resolved Hide resolved
@kmuehlbauer
Copy link
Contributor

@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

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

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

Thanks @zequihg50 and @kmuehlbauer

@dcherian dcherian merged commit 1411474 into pydata:main Nov 15, 2023
25 of 28 checks passed
@welcome 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

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)
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