Skip to content

Commit 9ffd109

Browse files
committed
docs(indexing): inherit current-contract docstrings from audit
Assisted-by: Codex:GPT-6
2 parents 692322c + 1e385a8 commit 9ffd109

12 files changed

Lines changed: 57 additions & 173 deletions

File tree

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
11
Correct indexing, reader, serialization, cache, and integration descriptions to match supported behavior; qualify NumPy/TensorStore compatibility and performance claims.
2+
3+
Describe current contracts in source and test docstrings instead of narrating prior implementations. Clarify that immutable index coordinates do not snapshot source values.

packages/zarr-indexing/src/zarr_indexing/boundary.py

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -210,11 +210,9 @@ def split_scalar_axes(
210210
scalar with the advanced indices for the purpose of placing the broadcast
211211
result, so the two disagree when a scalar and an index array are separated:
212212
`a[0, ..., [1, 2]]` has shape `(2, 3)` for a `(2, 3, 4)` array, where
213-
`a[0][..., [1, 2]]` has shape `(3, 2)`. The earlier claim here that they
214-
always agree rested on `a[0, [1, 2], :]`, where the indices are adjacent and
215-
they happen to. Scalar-first is the documented dialect (see the `lazy_array`
216-
module docstring) — the divergence is deliberate, and this note exists so
217-
that the correct end is not "fixed" later.
213+
`a[0][..., [1, 2]]` has shape `(3, 2)`. Scalar-first processing is the
214+
wrapper's indexing dialect; it does not implement NumPy's full advanced-axis
215+
placement rules.
218216
219217
Parameters
220218
----------

packages/zarr-indexing/src/zarr_indexing/domain.py

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -82,12 +82,10 @@ def __post_init__(self) -> None:
8282
def _unchecked(
8383
cls, inclusive_min: tuple[int, ...], exclusive_max: tuple[int, ...]
8484
) -> IndexDomain:
85-
"""Build an unlabeled domain from bounds the caller has already established.
85+
"""Build an unlabeled domain from validated bounds.
8686
87-
Skips `__post_init__`. For internal producers that derive bounds from an
88-
already-valid domain — chunk resolution builds two domains per chunk,
89-
and re-validating them was a measurable share of a plan's cost.
90-
"""
87+
Skip __post_init__; internal callers must ensure equal bound lengths
88+
and inclusive_min <= exclusive_max in every dimension."""
9189
domain = object.__new__(cls)
9290
object.__setattr__(domain, "inclusive_min", inclusive_min)
9391
object.__setattr__(domain, "exclusive_max", exclusive_max)
@@ -210,13 +208,9 @@ def narrow(self, selection: Any) -> IndexDomain:
210208
Raises
211209
------
212210
BoundsCheckError
213-
If a bound lies outside this domain. A slice bound used to be
214-
clamped instead, so `narrow(slice(-3, None))` on `[0, 10)` quietly
215-
returned the whole axis — reading as the NumPy spelling of "the last
216-
three" and answering with something else — and `narrow(slice(20,
217-
30))` returned a domain its own parent did not contain. The rest of
218-
the algebra states no clamping and no negative wrapping as an
219-
invariant and enforces it; this is the one place that did not.
211+
If an integer index or explicit slice bound lies outside this domain.
212+
Bounds are checked as literal coordinates, without clamping or
213+
negative-index wrapping.
220214
"""
221215
normalized = _normalize_selection(selection, self.ndim)
222216
new_inclusive_min: list[int] = []

packages/zarr-indexing/src/zarr_indexing/lazy_array.py

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -898,9 +898,6 @@ def with_parts(self, parts: Sequence[int]) -> LazyArray:
898898
[`with_parts_per_axis`][zarr_indexing.lazy_array.LazyArray.with_parts_per_axis],
899899
and to read in one pass see
900900
[`unpartitioned`][zarr_indexing.lazy_array.LazyArray.unpartitioned].
901-
The three were one parameter whose meaning was decided by inspecting the
902-
type of what it was given, which left no way to ask for one of them and
903-
be told when you had spelled it wrong.
904901
905902
Parameters
906903
----------

packages/zarr-indexing/src/zarr_indexing/output_map.py

Lines changed: 15 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -194,15 +194,12 @@ class ArrayMap:
194194
"""Multiplier applied to each `index_array` value before `offset` is added."""
195195

196196
def __post_init__(self) -> None:
197-
"""Own the index array and expose it read-only.
198-
199-
A map is frozen, but the array inside it was not: reaching through a
200-
view's transform to `index_array[0] = 9` silently changed what the view
201-
returned, in a package whose whole contract is that a view is a
202-
description of a read and resolving it twice answers alike. Owning the
203-
array also prevents the caller from changing the contents behind the
204-
read-only view, which would invalidate this value object's hash.
205-
"""
197+
"""Own an immutable snapshot of the integer index coordinates.
198+
199+
The snapshot is backed by immutable bytes, so callers cannot modify it
200+
or re-enable its WRITEABLE flag. Changes to the supplied array do not
201+
change the map's coordinates or hash. This freezes the coordinate
202+
mapping, not the source values read through it."""
206203
# Immutable bytes are the ultimate owner so callers cannot re-enable
207204
# the WRITEABLE flag, as they can on a read-only array that owns its
208205
# allocation. `asarray` also accepts the NumPy scalars that reach here
@@ -222,29 +219,21 @@ def __reduce__(self) -> tuple[object, tuple[object, int, int]]:
222219
)
223220

224221
def _with_affine(self, offset: int, stride: int) -> ArrayMap:
225-
"""This map's coordinates under a different affine adjustment.
222+
"""Return a map with a different affine adjustment.
226223
227-
The frozen index array is shared rather than copied: it is already
228-
owned by immutable bytes and read-only, so the ownership invariant
229-
`__post_init__` establishes holds for the new map too. Chunk
230-
resolution translates every restricted map once per chunk, and
231-
re-copying the array there dominated the cost of small selections.
232-
"""
224+
Share the immutable index array while replacing the offset and stride.
225+
This preserves coordinate ownership without copying the array."""
233226
new = object.__new__(ArrayMap)
234227
object.__setattr__(new, "index_array", self.index_array)
235228
object.__setattr__(new, "offset", offset)
236229
object.__setattr__(new, "stride", stride)
237230
return new
238231

239232
def __eq__(self, other: object) -> bool:
240-
"""Value equality, comparing index arrays element-wise.
233+
"""Compare offset, stride, array shape, and index values.
241234
242-
The generated `__eq__` compares them with `==`, whose result for two
243-
arrays is an array — so asking whether two maps are equal raised
244-
`ValueError: the truth value of an array ... is ambiguous`. `frozen=True`
245-
reads as a promise that a value can be compared and hashed, and this is
246-
what makes good on it.
247-
"""
235+
Return a scalar boolean for another ArrayMap and NotImplemented for
236+
other types."""
248237
if not isinstance(other, ArrayMap):
249238
return NotImplemented
250239
return (
@@ -255,11 +244,10 @@ def __eq__(self, other: object) -> bool:
255244
)
256245

257246
def __hash__(self) -> int:
258-
"""Hashed by the array's contents, so equal maps hash alike.
247+
"""Hash the offset, stride, array shape, and index bytes.
259248
260-
The generated `__hash__` hashed the ndarray itself, which is unhashable;
261-
a map could therefore not go in a set, or key a cache.
262-
"""
249+
The immutable coordinate snapshot keeps the hash stable, and equal
250+
maps have equal hashes."""
263251
return hash(
264252
(
265253
self.offset,

packages/zarr-indexing/src/zarr_indexing/testing/strategies.py

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -70,16 +70,11 @@ def _basic_entry(size: int) -> st.SearchStrategy[Any]:
7070

7171

7272
def _orthogonal_entry(size: int) -> st.SearchStrategy[Any]:
73-
"""One axis of an `oindex` selection.
73+
"""Generate one axis of an orthogonal selection.
7474
75-
The slices carry a step and are free to stop early. Drawing them as
76-
`slice(start, size)` alone meant no strided or reversed slice ever reached
77-
`oindex`, and no orthogonal selection ever stopped short of the axis end.
78-
79-
An empty coordinate list is drawn too. It selects nothing, which is legal
80-
and is exactly the shape that lost its axis on the way through JSON — but
81-
with `min_size=1` no fancy selection was ever empty.
82-
"""
75+
Include scalar coordinates, coordinate lists, boolean masks, and slices
76+
with positive or negative steps and varying endpoints. Empty coordinate
77+
lists and all-False masks exercise selections with zero-length axes."""
8378
coordinate = st.integers(-size, size - 1)
8479
return st.one_of(
8580
coordinate,
@@ -119,13 +114,10 @@ def masks(draw: st.DrawFn, shape: tuple[int, ...]) -> np.ndarray[Any, np.dtype[n
119114

120115

121116
def empty_masks(shape: tuple[int, ...]) -> st.SearchStrategy[np.ndarray[Any, np.dtype[np.bool_]]]:
122-
"""The all-False mask over `shape` — a fancy selection that empties the view.
117+
"""Generate an all-False mask with the given shape.
123118
124-
Split out from `masks`, which forces a cell True so a chain has something
125-
left to index at the next step. Drawn on its own because an empty fancy
126-
selection is a shape the code paths treat separately, and nothing generated
127-
one.
128-
"""
119+
This selects no elements. Unlike masks(), which includes a True cell,
120+
this strategy exercises empty fancy selections."""
129121
return st.just(np.zeros(shape, dtype=np.bool_))
130122

131123

packages/zarr-indexing/tests/test_composition.py

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -440,11 +440,7 @@ def test_composing_an_inner_array_with_a_broadcast_axis_wider_than_one_cell() ->
440440

441441

442442
def test_composing_a_one_dimensional_inner_array_under_a_higher_rank_outer() -> None:
443-
"""The shortcut gated on the output rank but sized by the input rank.
444-
445-
A rank-2 outer therefore built a rank-1 array for a rank-2 domain, which the
446-
engine's own invariant then rejected.
447-
"""
443+
"""Composed index arrays retain the full input rank of their domain."""
448444
outer = IndexTransform(
449445
domain=IndexDomain.from_shape((2, 3)),
450446
output=(DimensionMap(input_dimension=0, offset=1, stride=1),),

packages/zarr-indexing/tests/test_json.py

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -356,12 +356,7 @@ def _index_array_body(index_array: Any, rank: int = 1, extent: int = 2) -> Index
356356
ids=["floats", "mixed", "bools", "strings", "string", "scalar", "nulls"],
357357
)
358358
def test_a_non_integer_index_array_is_rejected(index_array: Any, detail: str) -> None:
359-
"""An `index_array` addresses output coordinates, so it must be integral.
360-
361-
Lowering a float array silently truncated it (`[0.9, 1.9]` selected cells 0
362-
and 1), a bool array coerced to 0/1, and a string array leaked a raw NumPy
363-
`ValueError` from the middle of the conversion.
364-
"""
359+
"""Index arrays require integer coordinates and reject float, bool, and string dtypes."""
365360
with pytest.raises(NdselError) as excinfo:
366361
IndexTransform.from_json(_index_array_body(index_array))
367362
assert excinfo.value.reason == "invalid_json"
@@ -546,13 +541,7 @@ def test_an_ambiguous_empty_index_array_is_rejected() -> None:
546541
ids=["float", "string", "bool", "non-string-label", "out-of-range"],
547542
)
548543
def test_a_malformed_domain_document_is_rejected(document: Any, reason: str, detail: str) -> None:
549-
"""The domain loader validates what the message layer validates.
550-
551-
Reading the keys directly was a second, undefended way into the same
552-
objects: a bare `int()` truncated `3.9` to 3, coerced `"3"` and `True`, and
553-
let a non-string label into a `tuple[str, ...]` — each building a domain
554-
that was not the document's, and re-dumping as a different document.
555-
"""
544+
"""The domain loader enforces coordinate types, integer bounds, and string labels."""
556545
with pytest.raises(NdselError) as excinfo:
557546
IndexDomain.from_json(document)
558547
assert excinfo.value.reason == reason

packages/zarr-indexing/tests/test_lazy_array.py

Lines changed: 11 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -276,11 +276,7 @@ def test_array_map_dependent_axis_reports_no_axis() -> None:
276276

277277

278278
def test_scalar_on_a_fancy_axis_collapses_to_a_constant() -> None:
279-
"""The degenerate-collapse rule: an all-singleton ArrayMap becomes a ConstantMap.
280-
281-
Without it the map keeps an `input_dimension` naming an axis the integer
282-
index just removed, which after renumbering aliases a different axis.
283-
"""
279+
"""Scalar selection collapses an all-singleton ArrayMap to a ConstantMap."""
284280
view = IndexTransform.from_shape((7, 5)).oindex[np.array([3, 1]), slice(None)][0]
285281
assert view.output[0] == ConstantMap(offset=3)
286282
assert isinstance(view.output[1], DimensionMap)
@@ -926,13 +922,10 @@ def test_a_boolean_mask_composes_onto_a_fancy_view() -> None:
926922

927923

928924
def test_an_ellipsis_only_vindex_step_preserves_a_correlated_gather() -> None:
929-
"""Regression: a slice-only vindex step misread correlated maps as orthogonal.
925+
"""Ellipsis-only vectorized selection preserves correlated coordinates.
930926
931-
`vindex[...]` (and `vindex[..., scalar]`, whose remainder after the scalar
932-
is split off is ellipsis-only) used to stamp each correlated map with its
933-
block axis as an orthogonal binding. Two "orthogonal" maps then shared one
934-
input axis, and the partition walk rejected its own transform mid-read.
935-
"""
927+
This also applies after scalar indices have been split off. The selected
928+
values must agree across partitionings."""
936929
base = np.arange(16).reshape(4, 4)
937930

938931
for parts in (None, (2, 2), (4, 4), (1, 3)):
@@ -2435,15 +2428,7 @@ def test_a_masked_source_keeps_its_mask_under_every_partitioning(parts: Any) ->
24352428

24362429
@pytest.mark.parametrize("parts", [None, (2, 2), (3, 4)])
24372430
def test_a_masked_source_keeps_its_mask_when_the_view_is_empty(parts: Any) -> None:
2438-
"""An empty result is still a result, and its type must not depend on the parts.
2439-
2440-
An empty view is answered without reading the source at all, and that
2441-
shortcut reached for the array namespace's own `empty` — which knows nothing
2442-
about masks — so an unpartitioned empty view came back a plain array while
2443-
the same view partitioned came back masked. No cells either way, so nothing
2444-
about the values changed; the caller just got a different type depending on
2445-
how the read had been divided.
2446-
"""
2431+
"""Empty views of masked sources return masked arrays for every partitioning."""
24472432
data = np.ma.masked_greater(np.arange(12).reshape(3, 4), 7)
24482433
got = repartition(LazyArray(data), parts).lazy[:, 2:2].result()
24492434
assert isinstance(got, np.ma.MaskedArray), parts
@@ -2453,11 +2438,7 @@ def test_a_masked_source_keeps_its_mask_when_the_view_is_empty(parts: Any) -> No
24532438
def test_a_large_array_without_dask_refuses_to_claim_equality(
24542439
monkeypatch: pytest.MonkeyPatch,
24552440
) -> None:
2456-
"""Above the digest limit the fallback must miss a cache rather than lie.
2457-
2458-
Two arrays differing in one element used to token identically, because the
2459-
fallback described the shape and dtype and gave up on the contents.
2460-
"""
2441+
"""Without Dask, arrays above the digest limit receive distinct fallback tokens."""
24612442
import sys
24622443

24632444
monkeypatch.setitem(sys.modules, "dask.base", None)
@@ -2521,15 +2502,7 @@ def test_a_zero_chunk_on_a_nonempty_axis_is_still_rejected() -> None:
25212502
def test_the_coverage_count_agrees_with_numpy_for_reversed_selections(
25222503
selection: tuple[Any, ...],
25232504
) -> None:
2524-
"""The safety net behind `result()`'s coverage assertion, checked on its own.
2525-
2526-
`_out_selection_cell_count` sizes a partition's `out_selection` without
2527-
materializing it, and `result()` trusts that count to decide whether the
2528-
walk covered the view. Nothing pinned it for a reversed slice, so dropping
2529-
its `start <= stop` guard — or wrapping the subtraction in `abs()` — left
2530-
the suite green. A net nobody tests only matters once something else breaks,
2531-
which is exactly when it needs to be right.
2532-
"""
2505+
"""Coverage counts match NumPy selection sizes for reversed slices."""
25332506
data = reference()
25342507
view = LazyArray(data).with_parts((2, 2, 2)).lazy[selection]
25352508
out_shape = view.shape
@@ -2556,26 +2529,14 @@ def test_the_coverage_count_agrees_with_numpy_for_reversed_selections(
25562529
def test_the_coverage_count_matches_numpy_for_intervals_the_fast_path_declines(
25572530
selection: tuple[Any, ...], out_shape: tuple[int, ...], expected: int
25582531
) -> None:
2559-
"""The guard on `result()`'s safety net, exercised where the walk cannot reach it.
2560-
2561-
A partition walk only ever produces concrete forward in-bounds intervals, so
2562-
the guard that keeps everything else off the subtraction fast path is not
2563-
reachable through `parts()` at all — which is why removing it left the whole
2564-
suite green. It is the net's own contract, so it is checked directly.
2565-
"""
2532+
"""Coverage counts match NumPy for intervals outside the subtraction fast path."""
25662533
counted = _out_selection_cell_count(selection, out_shape)
25672534
assert counted == np.empty(out_shape)[selection].size
25682535
assert counted == expected
25692536

25702537

25712538
def test_a_zero_dimensional_index_array_drops_its_axis_like_a_scalar() -> None:
2572-
"""`a[np.array(2), :]` is `a[2, :]` in NumPy, and now here too.
2573-
2574-
Only Python and NumPy integers counted as scalars, so a 0-d array fell
2575-
through to the fancy path and was widened into a length-1 index array —
2576-
keeping an axis NumPy drops. That was a third answer, agreeing with neither
2577-
NumPy nor eager zarr, which rejects it.
2578-
"""
2539+
"""Zero-dimensional integer arrays drop axes in orthogonal and vectorized selections."""
25792540
data = np.arange(20).reshape(4, 5)
25802541
for mode, expected in (
25812542
("oindex", data[np.array(2), :]),
@@ -2591,12 +2552,7 @@ def test_a_zero_dimensional_index_array_drops_its_axis_like_a_scalar() -> None:
25912552

25922553

25932554
def test_a_multidimensional_array_in_an_orthogonal_selection_is_refused() -> None:
2594-
"""The rule belongs to the selection, so the message speaks its vocabulary.
2595-
2596-
Left to the engine, this surfaced as a rank complaint about an `index_array`
2597-
the caller never wrote — the transform layer's words for a mistake made two
2598-
layers above it.
2599-
"""
2555+
"""Reject multidimensional orthogonal index arrays with a selection-level error."""
26002556
with pytest.raises(IndexError, match="must be 1-dimensional"):
26012557
LazyArray(np.arange(20).reshape(4, 5)).lazy.oindex[[[0, 1], [2, 3]], slice(None)]
26022558

@@ -2611,13 +2567,7 @@ def test_with_parts_rejects_a_bare_integer() -> None:
26112567

26122568

26132569
def test_fancy_composition_over_an_empty_axis() -> None:
2614-
"""Regression: composing fancy steps over an empty axis stays unpinned.
2615-
2616-
The empty-domain branch of `compose` produces index arrays that are
2617-
singleton on every non-empty axis; pinning one to an axis it merely
2618-
broadcasts along made a later basic step index a size-1 axis positionally
2619-
and raise, deep inside a legal chain.
2620-
"""
2570+
"""Fancy composition over an empty axis preserves shape through later selections."""
26212571
base = np.empty((3, 0, 6), dtype=np.int64)
26222572
view = LazyArray(base).lazy.oindex[[2, 1], :, [5, 0, 3]]
26232573
assert view.shape == (2, 0, 3)

packages/zarr-indexing/tests/test_lazy_array_stateful.py

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,9 @@
99
declared. The zarr case runs a smaller budget: it reads through a store, and it
1010
exercises the same code paths.
1111
12-
This replaces a seeded `_random_chain` sweep in `test_lazy_array` that read
13-
chained selections through `parts()`. That sweep did reach the states it was
14-
meant to, but a rank-0 correlated view was absorbed by a reshape in `result()`
15-
and mirrored into the sweep rather than read as a failure; asserting the
16-
documented assembly literally makes that impossible to paper over.
17-
`test_lazy_array` keeps its `result()`-based sweep, which is the deterministic
18-
cross-flavor coverage this does not attempt.
12+
The state machine checks both result() and explicit partition assembly against
13+
NumPy, including the shape of each partition's values. test_lazy_array also
14+
provides deterministic selection-chain coverage across source flavors.
1915
"""
2016

2117
from __future__ import annotations

0 commit comments

Comments
 (0)