-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Refactoring/fixing zarr-pyhton v3 incompatibilities in xarray datatrees #10020
Conversation
…n open_dtree for zarr
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…o dtree-zarrv3
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @aladinor for working on this! I'm happy to see that we are actually quite close to getting this working. I think the changes you made to the tests highlight that we have some work to do around consolidated metadata and possibly error handling in Zarr.
Co-authored-by: Joe Hamman <[email protected]>
This reverts commit 22ac9b4.
for more information, see https://pre-commit.ci
@requires_zarr | ||
@parametrize_zarr_format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We can also do this, though that fixture is in test_backends.py
:
xarray/xarray/tests/test_backends.py
Line 2281 in 2da7f55
@pytest.mark.usefixtures("default_zarr_format") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to consolidate, but is that implementation better in any way? I find it a lot harder to read...
zarr_format: Literal[2, 3], | ||
) -> None: | ||
"""For one zarr array, check that all expected metadata and chunk data files exist.""" | ||
# TODO: This function is now so complicated that it's practically checking compliance with the whole zarr spec... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I think the better test here is to write some dask arrays with random data setting compute=False
and then assert that you only get fill_value on read. That checks that real data is not written with compute=False
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just punt this to a follow-up PR? All the detailed checks here are so subtle that I'm worried if I try to refactor it again I'm going to either miss things or just delay this PR much further, and ultimately this works fine.
Co-authored-by: Deepak Cherian <[email protected]>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good lets iterate on a future PR.
Thanks @aladinor @TomNicholas @jhamman !
* main: Refactoring/fixing zarr-python v3 incompatibilities in xarray datatrees (pydata#10020) Refactor calendar fixtures (pydata#10150) Use flox for grouped first, last. (pydata#10148) Update flaky pydap test (pydata#10149)
whats-new.rst