Skip to content

Fix missing case to fully revert change to default write_empty_chunks. #1005

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

Conversation

tomwhite
Copy link
Contributor

@tomwhite tomwhite commented Apr 6, 2022

Fix issue identified in #1001 (comment). I have manually tested this change with:

>>> 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', '0.0', '0.1']

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@jakirkham
Copy link
Member

Thanks Tom! 😄

What is the result of git --no-pager grep 'write_empty_chunks=False'?

@tomwhite
Copy link
Contributor Author

tomwhite commented Apr 6, 2022

Thanks Tom! 😄

What is the result of git --no-pager grep 'write_empty_chunks=False'?

Docs and tests - so looks good to me!

docs/tutorial.rst:When creating an array with ``write_empty_chunks=False``, 
docs/tutorial.rst:    write_empty_chunks=False:
zarr/tests/test_core.py:        z = self.create_array(shape=(), dtype=a.dtype, fill_value=0, write_empty_chunks=False)
zarr/tests/test_core.py:                              write_empty_chunks=False)

@jakirkham jakirkham requested a review from joshmoore April 6, 2022 16:45
Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

👍 thanks all. Doing a quick 2.11.3 release before 🛏️, but I'll add:

  1. I knew we should have made the default a constant!
  2. During the community call, @d-v-b offered to open a follow-on PR (post-2.11.3) to propose an "auto" setting for write_empty_chunks.

@joshmoore joshmoore merged commit 66f29e3 into zarr-developers:2_11 Apr 6, 2022
@jakirkham
Copy link
Member

Yeah should have checked git grep before 🤦‍♂️

Anyways this is now being added to conda-forge too ( conda-forge/zarr-feedstock#61 ). Thanks for kicking this off Josh and Tom for the PR 🙏

@joshmoore
Copy link
Member

joshmoore commented Apr 7, 2022

Yeah should have checked git grep before 🤦‍♂️

I realized while trying to forward port this to master that I introduced this in #1002 since the diff had an extra newline! So we would have needed to re-grep #1002!

Mea culpa!

@tomwhite
Copy link
Contributor Author

tomwhite commented Apr 7, 2022

Thanks @joshmoore - 2.11.3 seems to be working now!

@joshmoore
Copy link
Member

whew 😓

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.

3 participants