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

Encode coords for zarr router #178

Merged

Conversation

mpiannucci
Copy link
Contributor

See #175

Previously, the coordinates of each DataArray were not encoded, only the dims were. So we couldnt plot this how it is shown below.

Tested with:

# We can access our API using fsspec's HTTPFileSystem
fs = HTTPFileSystem()

# The http mapper gives us a dict-like interface to the API
http_map = fs.get_mapper("http://0.0.0.0:8090/datasets/dbofs")

ds = xr.open_zarr(http_map, consolidated=True)
ds.temp.isel(ocean_time=0, s_rho=0).plot(x='lon_rho', y='lat_rho', cmap='jet')

image

xpublish/utils/zarr.py Outdated Show resolved Hide resolved
Copy link
Member

@abkfenris abkfenris left a comment

Choose a reason for hiding this comment

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

It would also be nice to get a test that captures the current loss of the coords. It could also help comparing if the change in extracting attrs from the un/encoded data array is going to cause issues.

xpublish/utils/zarr.py Outdated Show resolved Hide resolved
@mpiannucci
Copy link
Contributor Author

The only thing missing is a test case. I will circle back to that, probably have to modify the create_dataset function in the test utils to make something with differing coords and dims.

@mpiannucci mpiannucci requested a review from abkfenris April 6, 2023 19:03
@mpiannucci
Copy link
Contributor Author

Added a simple test case, lemme know if we need something more comprehensive

Copy link
Member

@abkfenris abkfenris left a comment

Choose a reason for hiding this comment

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

That looks good to me. Thanks Matt!

@abkfenris abkfenris merged commit 9e56f09 into xpublish-community:main Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants