Skip to content

Backends descriptions #7200

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

Merged
merged 5 commits into from
Oct 26, 2022
Merged

Backends descriptions #7200

merged 5 commits into from
Oct 26, 2022

Conversation

headtr1ck
Copy link
Collaborator

@github-actions github-actions bot added io topic-backends topic-zarr Related to zarr storage library labels Oct 23, 2022
Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Great!

@@ -353,6 +353,8 @@ def close(self, **kwargs):

class H5netcdfBackendEntrypoint(BackendEntrypoint):
available = has_h5netcdf
description = "Open netCDF files (.nc, .nc4 and .cdf) using h5netcdf in Xarray"
url = "https://docs.xarray.dev/en/stable/generated/xarray.backends.H5netcdfBackendEntrypoint.html"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting idea. I wonder whether these should be in docstrings then be linked by Sphinx, vs. in code. But ofc fine to try from my perspective.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just one comment here, h5netcdf (and netcdf4) can also open hdf5 files which are not strict NetCDF4. Not sure how to better phrase this, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting idea. I wonder whether these should be in docstrings then be linked by Sphinx, vs. in code. But ofc fine to try from my perspective.

No idea how that would work, could you give an example?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just one comment here, h5netcdf (and netcdf4) can also open hdf5 files which are not strict NetCDF4. Not sure how to better phrase this, though.

Should ".h5" be added to guess_can_open?
Or you don't want to specify this because not strict netCDF files might not work with xarray?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should ".h5" be added to guess_can_open?

I'd say that this is not necessary. Those who want to open will use the engine-kwarg.

There are only a few hdf5-specifics which can't be read by netcdf4 (see https://h5netcdf.org/#invalid-netcdf-files, list might not be up-to-date as netcdf-c is evolving too). So xarray is able digest most normal hdf5-files (via netcdf4 and h5netcdf).

My concern is, that users might conclude that only netCDF4-files are readable and get confused. Good chance that my impression will not hold and users know what they are doing.

Anyway, I really like these enhancements to the backends.

@headtr1ck
Copy link
Collaborator Author

If one wants to get fancy, we could wrap the list_engines dict into a custom dict with a nice __repr__ (maybe even html repr).

@TomNicholas
Copy link
Member

TomNicholas commented Oct 24, 2022

If one wants to get fancy, we could wrap the list_engines dict into a custom dict with a nice repr (maybe even html repr).

Whilst I don't want to discourage eager devs, in one of the community calls we did discuss how this would almost certainly be overkill for a little-used advanced feature.

@headtr1ck
Copy link
Collaborator Author

Since the url points to our internal documentation I assume it would be beneficial to add some docstrings to the Entrypoint classes?

@headtr1ck headtr1ck added the plan to merge Final call for comments label Oct 25, 2022
@dcherian dcherian merged commit 0a75d13 into pydata:main Oct 26, 2022
@headtr1ck headtr1ck deleted the backends branch October 26, 2022 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
io plan to merge Final call for comments topic-backends topic-zarr Related to zarr storage library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backend entrypoints not public?
5 participants