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.

Sign up for GitHub

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

Jump to bottom

Deterministic chunk padding #2755

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Deterministic chunk padding #2755

wants to merge 9 commits into from
+31 −4

Conversation

brokkoli71
Copy link
Member

@brokkoli71 brokkoli71 commented Jan 23, 2025 •
edited
Loading

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/user-guide/*.rst
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@brokkoli71 brokkoli71 marked this pull request as ready for review January 23, 2025 15:04
dstansby
dstansby reviewed Jan 24, 2025
View reviewed changes
Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

A couple of suggestions (in particular, explaining in a bit more detail the user facing bug this fixes), otherwise looks good!

changes/2755.bugfix.rst
@@ -0,0 +1 @@
Upon creation, an empty ``zarr.core.buffer.cpu.NDBuffer`` is filled with zeros to ensure deterministic behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Upon creation, an empty ``zarr.core.buffer.cpu.NDBuffer`` is filled with zeros to ensure deterministic behavior.
Upon creation, an empty ``zarr.core.buffer.cpu.NDBuffer`` is filled with zeros to ensure deterministic behavior.
This fixes a bug where Zarr format 2 data with no fill value was written with un-predictable chunk sizes.

tests/test_store/test_memory.py Outdated Show resolved Hide resolved
dstansby
dstansby requested changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Marking this as request changes, since it looks like it broke some tests - I had a look at the tests, and failure seems real (requesting a fill value of "", and getting 0 instead)

@normanrz normanrz enabled auto-merge (squash) January 24, 2025 13:44
@normanrz
Copy link
Member

Marking this as request changes, since it looks like it broke some tests - I had a look at the tests, and failure seems real (requesting a fill value of "", and getting 0 instead)

Fixed it.

dstansby
dstansby requested changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@dstansby dstansby left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If this now means zarr.emtpy deafults to filling data with zeros, instead of undefined data, this is a behaviour change from zarr-python 2: https://zarr.readthedocs.io/en/v2.18.4/api/creation.html#zarr.creation.empty - I think this is fine, but we should document that change in the changelog.

The docstring of zarr.creation.empty is now also incorrect, and needs updating:

The contents of an empty Zarr array are not defined. On attempting to retrieve data from an empty Zarr array, any values may be returned, and these are not guaranteed to be stable from one access to the next.

I also had a suggested improvement to the changelog above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhamman jhamman jhamman left review comments

@normanrz normanrz normanrz left review comments

@dstansby dstansby dstansby requested changes

@d-v-b d-v-b d-v-b approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Chunk padding is non-deterministic with zarr_format=2
5 participants
@brokkoli71 @normanrz @jhamman @d-v-b @dstansby