Skip to content

Expose use_cftime option in open_zarr #2886 #3229

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 7 commits into from
Sep 2, 2020

Conversation

Geektrovert
Copy link
Contributor

@Geektrovert Geektrovert commented Aug 19, 2019

Copy link
Member

@spencerkclark spencerkclark left a comment

Choose a reason for hiding this comment

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

Thanks @Geektrovert - for testing I think we need to create a temporary zarr store that contains a single time that could be decoded into a different date type depending on the value passed to use_cftime. Then we need to assert that the time gets decoded to the date type we expect when we use open_zarr with different values of use_cftime.

For details on how to create a temporary zarr store you could probably draw some inspiration from looking at some of the other tests in test_backends.py, like this one:

def test_append_write(self):
ds, ds_to_append, _ = create_append_test_data()
with self.create_zarr_target() as store_target:
ds.to_zarr(store_target, mode="w")
ds_to_append.to_zarr(store_target, append_dim="time")
original = xr.concat([ds, ds_to_append], dim="time")
assert_identical(original, xr.open_zarr(store_target))

@andersy005
Copy link
Member

andersy005 commented Aug 25, 2020

@Geektrovert, thank you for working on this! I rebased your fork to upstream master and added some tests locally. Do you mind giving me permissions so that I can push to your fork so as to bring this PR to completion?

@Geektrovert
Copy link
Contributor Author

@andersy005 sure! I will add you as a collaborator to my fork. Thank you for working on this!

Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

LGTM. This just needs a whats-new entry

Copy link
Member

@spencerkclark spencerkclark left a comment

Choose a reason for hiding this comment

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

Indeed this looks great @andersy005; thanks for finishing this up.

@andersy005
Copy link
Member

LGTM. This just needs a whats-new entry

Done! I don't know why the readthedocs build failed though

@keewis keewis closed this Sep 2, 2020
@keewis keewis reopened this Sep 2, 2020
@keewis
Copy link
Collaborator

keewis commented Sep 2, 2020

this seems to be a general issue with RTD (there are quite a few open issues about this already) and has to be fixed by them.

@dcherian
Copy link
Contributor

dcherian commented Sep 2, 2020

Thanks @Geektrovert & @andersy005. This is a great improvement.

@Geektrovert I see this is your first PR. Thanks! and welcome to xarray.

@dcherian dcherian merged commit 9ee0f01 into pydata:master Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-backends topic-zarr Related to zarr storage library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose use_cftime option in open_zarr
5 participants