Skip to content

fix: preserve non-canonical NaN payloads and distinguish -0.0 in v3 metadata - #4486

Open
barlowa124 wants to merge 9 commits into
zarr-developers:mainfrom
barlowa124:float-fill-4453
Open

barlowa124 wants to merge 9 commits into
zarr-developers:mainfrom
barlowa124:float-fill-4453

Conversation

@barlowa124

@barlowa124 barlowa124 commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Third item of #4453: float fill values holding non-canonical NaN
payloads did not round-trip, and ArrayV3Metadata.__eq__ conflated
documents whose fill_values serialize differently.

Parse to serialize dropped NaN payloads. "0x7fc00001" parsed fine,
but float_to_json_v3 flattened every NaN to "NaN" on write. It now
emits the hex bit pattern for any NaN differing from the canonical
positive one, which is the case the spec's hex encoding names. The old
comment claiming NumPy cannot hold distinct NaNs was wrong. The
payload bits were being discarded on write, not absent. The canonical
check compares all NaN bits including sign, matching zarrs and
tensorstore. In the same place, float_from_json_v3 decoded hex into
Python's 64-bit float and narrowed float16/float32 payloads. Hex
now decodes through np.frombuffer into a same-width scalar, so all
three widths round-trip.

Equality conflated distinct documents. __eq__ compared to_dict()
dictionaries, where negative zero equals positive zero and every NaN
equaled "NaN". __eq__ now compares json.dumps(to_dict(), sort_keys=True). That is the string __hash__ already hashes, so
equal implies equal-hash by construction.

For reviewers

float_from_json_v3 returns np.floating for hex input rather than
float. Both call sites consume it through a cast or complex(), so
callers see no change. "NaN" still denotes the canonical positive
NaN per width. v2 serialization is untouched. Complex fills route
through the same helpers per component, so a complex64 real part with
a non-canonical payload now round-trips as hex as well. That path
was exercised but has no dedicated test. Payload bits survive only when the hex
width matches the dtype width, which the spec requires anyway.

Author attestation

  • I am a human. These are my changes. I have reviewed and understood every change and can explain why each is correct.

TODO

Done:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • Changes documented as a new file in changes/

Outstanding:

  • New/modified features documented in docs/user-guide/*.md (no user-facing behavior beyond spec compliance)
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

…etadata

float_from_json_v3 decoded hex strings through Python float64, which
cannot carry a float16 payload and risks narrowing float32 payloads;
decode into a same-width NumPy scalar via np.frombuffer instead.
float_to_json_v3 flattened every NaN to "NaN" on write; emit the hex
bit pattern for NaNs that are not the canonical positive NaN so the
document round-trips the payload it parsed.

ArrayV3Metadata.__eq__ compared to_dict() dictionaries, under which
-0.0 == 0.0 and (before this change) any NaN payload == "NaN" even
though the documents serialize differently. Compare the canonical
serialized JSON text instead, which is also the exact form __hash__
already hashes, making the two consistent by construction.

Partial fix for zarr-developers#4453

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@github-actions github-actions Bot added the needs release notes Automatically applied to PRs which haven't added release notes label Oct 8, 2026
Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@github-actions github-actions Bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Oct 8, 2026
barlowa124 and others added 3 commits October 8, 2026 00:45
Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The strict bits==canonical check serialized float32 0xffc00000 as hex,
but the canonical NaN designation covers the payload, not the sign; mask
the sign bit so 0xffc00000 writes as "NaN" like the canonical form.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@d-v-b

d-v-b commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

thanks for this, I will have a look in the next few days!

@codecov

codecov Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.71%. Comparing base (19e84dc) to head (d238278).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4486      +/-   ##
==========================================
+ Coverage   94.69%   94.71%   +0.01%     
==========================================
  Files          94       94              
  Lines       13619    13666      +47     
==========================================
+ Hits        12897    12944      +47     
  Misses        722      722              
Files with missing lines Coverage Δ
src/zarr/core/dtype/npy/common.py 99.23% <100.00%> (+0.04%) ⬆️
src/zarr/core/metadata/v3.py 96.91% <100.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@d-v-b d-v-b left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. Compare all of a NaN's bits, sign included, before writing "NaN". Any other NaN, such as 0xffc00000, should be written as hex, as zarrs and tensorstore do. At the moment 0xffc00000 writes "NaN" and reads back as 0x7fc00000.
  2. Make ArrayV3Metadata.__eq__ work without json.dumps(..., sort_keys=True). It raises TypeError when attribute keys mix int and str, and that reaches Array.__eq__ too.
  3. Get the canonical NaN bits from numpy (np.asarray(np.nan, dtype=...) viewed as the matching uint) instead of hardcoded tables.
  4. Use parametrize in test_noncanonical_nan_serializes_as_hex, and add tests for the 0xffc00000 round trip and the complex path.
  5. note in the changelog that equality now compares the JSON text, so 1 and 1.0 in attributes no longer compare equal.

@barlowa124

Copy link
Copy Markdown
Contributor Author

Pushed 8a2714a.

"NaN" now requires the value's full bit pattern to equal the canonical NaN, sign bit included, so 0xffc00000 round-trips as "0xffc00000" in line with zarrs and tensorstore. The canonical pattern is read from np.asarray(np.nan, dtype=data.dtype).view(uint_dtype). __eq__ and __hash__ no longer pass sort_keys to json.dumps, and attribute dicts mixing int and str keys compare without raising TypeError. The hex-payload test is parametrized over six cases covering both sign-set payloads, and a Complex64 test covers the complex path. The changelog states that equality compares the JSON text and that 1 versus 1.0 attributes compare unequal.

python -m pytest tests/test_metadata/ tests/test_dtype/ gives 2200 passed.

@barlowa124

Copy link
Copy Markdown
Contributor Author

d238278 fixes the CI fallout: test_special_complex_fill_values_roundtrip expected inf * 1j to write "NaN" for its real part, but 0 * inf produces a NaN whose sign bit is platform-dependent (0xffc00000 on x86), so the expectation is now derived from the produced scalar's bits. ruff format applied on both branches.

This branch has not been deployed

No deployments
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.

2 participants