diff --git a/changes/4486.bugfix.md b/changes/4486.bugfix.md new file mode 100644 index 0000000000..ad6752b628 --- /dev/null +++ b/changes/4486.bugfix.md @@ -0,0 +1,11 @@ +Preserve non-canonical NaN payloads in v3 fill values. Only the exact +canonical NaN bit pattern, sign bit included, serializes as `NaN`; every +other NaN, including the negative-sign form of the canonical payload, +writes back as its hex bits, and hex strings decode into same-width +NumPy scalars so float16 payloads survive. `ArrayV3Metadata` equality +now compares the serialized JSON text without sorting keys, so attribute +dictionaries with mixed key types no longer raise `TypeError`; as a +consequence of comparing JSON text, attributes that differ only in JSON +spelling, such as `1` and `1.0`, no longer compare equal. This also +stops equality from conflating `-0.0` with `0.0`. Partial fix for issue +#4453. diff --git a/src/zarr/core/dtype/npy/common.py b/src/zarr/core/dtype/npy/common.py index f413f5f678..d2b6904565 100644 --- a/src/zarr/core/dtype/npy/common.py +++ b/src/zarr/core/dtype/npy/common.py @@ -1,7 +1,6 @@ from __future__ import annotations import base64 -import struct import sys from collections.abc import Sequence from typing import ( @@ -15,6 +14,7 @@ SupportsIndex, SupportsInt, TypeGuard, + cast, ) import numpy as np @@ -162,7 +162,7 @@ def float_from_json_v2(data: JSONFloatV2) -> float: return float(data) -def float_from_json_v3(data: JSONFloatV3) -> float: +def float_from_json_v3(data: JSONFloatV3) -> float | np.floating[Any]: """ Convert a JSON float to a float (v3). @@ -182,6 +182,9 @@ def float_from_json_v3(data: JSONFloatV3) -> float: "...for float32, "NaN" is equivalent to "0x7fc00000". This representation is the only way to specify a NaN value other than the specific NaN value denoted by "NaN"." + + Hex strings decode into a NumPy scalar of the matching width rather than a + Python float, so a non-canonical NaN payload survives the parse. """ if isinstance(data, str): @@ -194,18 +197,21 @@ def float_from_json_v3(data: JSONFloatV3) -> float: ) raise ValueError(msg) if len(data[2:]) == 4: - dtype_code = ">e" + dtype_code = ">f2" elif len(data[2:]) == 8: - dtype_code = ">f" + dtype_code = ">f4" elif len(data[2:]) == 16: - dtype_code = ">d" + dtype_code = ">f8" else: msg = ( f"Invalid hexadecimal float value: {data!r}. " "Expected the '0x' prefix to be followed by 4, 8, or 16 numeral characters" ) raise ValueError(msg) - return float(struct.unpack(dtype_code, bytes.fromhex(data[2:]))[0]) + return cast( + np.floating[Any], + np.frombuffer(bytes.fromhex(data[2:]), dtype=dtype_code)[0], + ) return float_from_json_v2(data) @@ -287,9 +293,23 @@ def float_to_json_v3(data: float | np.floating[Any]) -> JSONFloatV3: ------- JSONFloat The JSON representation of the float. - """ - # v3 can in principle handle distinct NaN values, but numpy does not represent these explicitly - # so we just reuse the v2 routine here + + Notes + ----- + ``"NaN"`` denotes exactly the canonical NaN bit pattern, sign bit + included. Any other NaN, including the negative-sign form of the + canonical payload, is written as its hexadecimal bit pattern per the + spec's hex-bits encoding (``"0x7fc00001"`` for a float32 NaN with + payload 1), which is the only way to name a non-canonical NaN. + """ + if np.isnan(data) and isinstance(data, np.floating): + uint_dtype = {2: np.uint16, 4: np.uint32, 8: np.uint64}.get(data.dtype.itemsize) + if uint_dtype is not None: + bits = int(np.asarray(data).view(uint_dtype)) + canonical = int(np.asarray(np.nan, dtype=data.dtype).view(uint_dtype)) + if bits == canonical: + return float_to_json_v2(data) + return f"0x{bits:0{data.dtype.itemsize * 2}x}" return float_to_json_v2(data) diff --git a/src/zarr/core/metadata/v3.py b/src/zarr/core/metadata/v3.py index 2d5d169e6b..176c7834f1 100644 --- a/src/zarr/core/metadata/v3.py +++ b/src/zarr/core/metadata/v3.py @@ -756,14 +756,17 @@ def __eq__(self, other: object) -> bool: # The default dataclass __eq__ compares fields directly, which is wrong for a NaN # fill_value: NaN != NaN under IEEE 754. Comparing the JSON-serialized form instead # treats matching NaN (and inf) fill values as equal. See issue #2929. + # The comparison uses the serialized JSON *text*: dict equality conflates + # -0.0 and 0.0, and a non-canonical NaN payload with "NaN", though the + # documents write back differently. See issue #4453. if not isinstance(other, ArrayV3Metadata): return NotImplemented - return self.to_dict() == other.to_dict() + return json.dumps(self.to_dict()) == json.dumps(other.to_dict()) def __hash__(self) -> int: # Hash the JSON-serialized form to stay consistent with __eq__: equal metadata # must hash equally, which a field-based hash violates for a NaN fill_value. - return hash(json.dumps(self.to_dict(), sort_keys=True)) + return hash(json.dumps(self.to_dict())) def update_shape(self, shape: tuple[int, ...]) -> Self: chunk_grid = self.chunk_grid diff --git a/tests/test_array.py b/tests/test_array.py index f78b895bf9..b90aa6bdc8 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -1037,17 +1037,26 @@ def test_write_empty_chunks_negative_zero( (np.nan * 1j, ["NaN", "NaN"]), (np.nan, ["NaN", 0.0]), (np.inf, ["Infinity", 0.0]), - (np.inf * 1j, ["NaN", "Infinity"]), + (np.inf * 1j, None), (-np.inf, ["-Infinity", 0.0]), (math.inf, ["Infinity", 0.0]), ], ) -async def test_special_complex_fill_values_roundtrip(fill_value: Any, expected: list[Any]) -> None: +async def test_special_complex_fill_values_roundtrip( + fill_value: Any, expected: list[Any] | None +) -> None: store = MemoryStore() zarr.create_array(store=store, shape=(1,), dtype=np.complex64, fill_value=fill_value) content = await store.get("zarr.json", prototype=default_buffer_prototype()) assert content is not None actual = json.loads(content.to_bytes()) + if expected is None: + # inf * 1j computes 0 * inf for the real part, which produces a NaN + # whose sign bit is platform-dependent. Only the exact canonical bits + # serialize as "NaN"; any other NaN writes as hex. + real_bits = int(np.array([np.complex64(fill_value)]).view(np.uint32)[0]) + canonical = int(np.asarray(np.nan, dtype=np.float32).view(np.uint32)) + expected = ["NaN" if real_bits == canonical else f"0x{real_bits:08x}", "Infinity"] assert actual["fill_value"] == expected diff --git a/tests/test_dtype/test_npy/test_float.py b/tests/test_dtype/test_npy/test_float.py index c8418f922e..79f4f6212e 100644 --- a/tests/test_dtype/test_npy/test_float.py +++ b/tests/test_dtype/test_npy/test_float.py @@ -1,9 +1,12 @@ from __future__ import annotations +from typing import Any + import numpy as np +import pytest from tests.test_dtype.test_wrapper import BaseTestZDType -from zarr.core.dtype.npy.float import Float16, Float32, Float64 +from zarr.core.dtype.npy.float import BaseFloat, Float16, Float32, Float64 class _BaseTestFloat(BaseTestZDType): @@ -223,3 +226,37 @@ def test_string_float_from_json_scalar() -> None: # Test that it works for v2 format too result = dtype_instance.from_json_scalar("1.5", zarr_format=2) assert result == np.float32(1.5) + + +@pytest.mark.parametrize( + ("dtype", "hex_value"), + [ + (Float16(), "0x7fc1"), + (Float32(), "0x7fc00001"), + (Float32(), "0xffc00000"), + (Float32(), "0xffc00001"), + (Float64(), "0x7ff8000000000001"), + (Float64(), "0xfff8000000000000"), + ], +) +def test_noncanonical_nan_serializes_as_hex(dtype: BaseFloat[Any, Any], hex_value: str) -> None: + """Only the exact canonical NaN bit pattern writes back as ``"NaN"``; + every other NaN, including the negative-sign form of the canonical + payload, writes back as its hex bits. Regression test for + https://github.com/zarr-developers/zarr-python/issues/4453 item 3.""" + scalar = dtype.from_json_scalar(hex_value, zarr_format=3) + assert dtype.to_json_scalar(scalar, zarr_format=3) == hex_value + canonical = dtype.from_json_scalar("NaN", zarr_format=3) + assert dtype.to_json_scalar(canonical, zarr_format=3) == "NaN" + + +def test_noncanonical_nan_serializes_as_hex_complex() -> None: + """The complex path applies the same canonical-NaN check to each part.""" + from zarr.core.dtype.npy.complex import Complex64 + + dtype = Complex64() + real_nan = np.array([0xFFC00000], dtype=np.uint32).view(np.float32)[0] + scalar = np.complex64(complex(real_nan, 0)) + assert dtype.to_json_scalar(scalar, zarr_format=3) == ("0xffc00000", 0.0) + canonical = np.complex64(complex(np.nan, np.nan)) + assert dtype.to_json_scalar(canonical, zarr_format=3) == ("NaN", "NaN") diff --git a/tests/test_metadata/test_v3.py b/tests/test_metadata/test_v3.py index 94240c36b9..8687888a6d 100644 --- a/tests/test_metadata/test_v3.py +++ b/tests/test_metadata/test_v3.py @@ -491,3 +491,31 @@ def test_group_metadata_to_dict_consolidated(attributes: dict[str, Any] | None) }, }, } + + +def test_eq_distinguishes_signed_zero_fill_value() -> None: + """Documents with fill_value -0.0 and 0.0 write back differently and must + not compare equal. Regression test for issue #4453 item 3.""" + neg = ArrayV3Metadata.from_dict( + minimal_metadata_dict_v3(data_type="float64", fill_value=-0.0) # type: ignore[arg-type] + ) + pos = ArrayV3Metadata.from_dict( + minimal_metadata_dict_v3(data_type="float64", fill_value=0.0) # type: ignore[arg-type] + ) + assert neg != pos + assert hash(neg) != hash(pos) + + +def test_nan_payload_fill_value_round_trips() -> None: + """A non-canonical NaN payload survives a document round trip as its hex + bits, and does not compare equal to the canonical NaN document.""" + payload = ArrayV3Metadata.from_dict( + minimal_metadata_dict_v3(data_type="float32", fill_value="0x7fc00001") # type: ignore[arg-type] + ) + canonical = ArrayV3Metadata.from_dict( + minimal_metadata_dict_v3(data_type="float32", fill_value="NaN") # type: ignore[arg-type] + ) + assert payload.to_dict()["fill_value"] == "0x7fc00001" + assert canonical.to_dict()["fill_value"] == "NaN" + assert payload != canonical + assert hash(payload) != hash(canonical)