-
-
Notifications
You must be signed in to change notification settings - Fork 329
Revert change to default write_empty_chunks. #1001
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
Revert change to default write_empty_chunks. #1001
Conversation
Great to see this, thanks @vyasr! cc @benjeffery @tomwhite |
I assume the general consensus is to get this out as an immediate 2.11.2? |
That's my understanding! |
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, @vyasr!
Merging and I'll start preparing a v2
branch for releasing from. (As a side note: slightly surprised that none of the tests needed updating. I assume that means that none are testing the default value, eh?)
Thanks all! 😄 |
* Activate GHA for stable 2_11 branch * Revert change to default write_empty_chunks. (#1001) * Prepare 2.11.2 release Co-authored-by: Vyas Ramasubramani <[email protected]>
2.11.2 is released from the |
Should we create a GitHub release for that tag as well? |
Ah, apologies. I forgot that step. Yes, I'll do it now. |
Not at all. Thanks for handling this Josh 🙏 |
Thanks for releasing 2.11.2 @joshmoore. Unfortunately, it looks like it doesn't fully revert the change: >>> import zarr
>>> zarr.__version__
'2.11.2'
>>> a = zarr.create((100, 100), chunks=(100, 50), dtype="i4", store="example.zarr")
>>> a[:] = 0
>>> import os
>>> os.listdir("example.zarr")
['.zarray'] Whereas with a previous version of Zarr (and the main branch): >>> import zarr
>>> zarr.__version__
'2.10.3'
>>> a = zarr.create((100, 100), chunks=(100, 50), dtype="i4", store="example.zarr")
>>> a[:] = 0
>>> import os
>>> os.listdir("example.zarr")
['.zarray', '0.0', '0.1'] The problem is in Line 23 in 04519bd
|
@tomwhite would you be willing to send a PR? 🙂 |
Opened #1005 |
Ah sorry I guessed I missed one, sorry about that. Thanks for the patch @tomwhite! |
This PR resolves #965 by changing the default of
write_empty_chunks
toTrue
. I am not sure if we want to document the current limitations ofwrite_empty_chunks=False
, or if we want to treat those as bugs that will be fixed eventually. In the future we may want to error if a user setswrite_empty_chunks=True
with an unsupported data type, but that change should probably be made in a future PR that implements the solution described in #965 (comment) and #965 (comment)TODO: