fix: reject a string-valued zero scale in the scale_offset codec - #4279
Merged
d-v-b merged 1 commit intoAug 22, 2026
Merged
Conversation
ScaleOffset.validate compared self.scale against 0 before parsing it. scale is documented and typed as int | float | str, and no string is ever == 0, so "0", "0.0" and the spec's hex form walked past the guard. On float dtypes that wrote every chunk as zero and read it back as NaN with no error; on integer dtypes it surfaced as an unhandled ZeroDivisionError. Compare the parsed scalar instead.
dylanpulver
force-pushed
the
fix-scale-offset-string-zero
branch
from
August 21, 2026 20:17
9292f76 to
749e970
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4279 +/- ##
=======================================
Coverage 94.12% 94.12%
=======================================
Files 92 92
Lines 12830 12831 +1
=======================================
+ Hits 12076 12077 +1
Misses 754 754
🚀 New features to boost your workflow:
|
Contributor
|
looks good, thanks for the fix and sorry about the bug |
d-v-b
approved these changes
Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ScaleOffset.validatecomparedself.scaleagainst0before parsing it.scaleis documented and typed asint | float | str, and no Python string equals0, so"0"and"0.0"walked past the guard, as did the spec's hex form"0x0000000000000000".On float dtypes the array is created, every chunk is written as zero and read back as NaN with no error. The zero scale is persisted into
zarr.json, so reopening the store reproduces it. On integer dtypes it surfaces as an unhandledZeroDivisionErrorinstead of theValueErrorthe guard promises.The comparison now runs on the value returned by
from_json_scalar. That reorders one thing: given a badoffsetand a zeroscaletogether, the representability error fires first. Nothing asserted the old order.Narrowing the annotation to drop
strwould also close it, but a stringscaleis a documented input and the suite already exercisesoffset="NaN".Residual:
scale="NaN"andscale=float("inf")are still accepted on float dtypes and also produce unusable reads. That reaches purely numeric input as well, so it reads as a separate question.For reviewers
The reorder is the only behavior change beyond the new rejection.
test_rejects_zero_scaleis now parametrized; the three string cases fail onmainand pass here, and the two numeric cases pass either way.Full suite: 6908 passed, 461 skipped, 4 xfailed. ruff 0.16.0 check and format clean. mypy 2.3.0 clean.
Author attestation
TODO
docs/user-guide/*.mdchanges/