feat(zarr-indexing): factor chunk plans into a columnar GridPartition - #316
Open
d-v-b wants to merge 7 commits into
Open
feat(zarr-indexing): factor chunk plans into a columnar GridPartition#316d-v-b wants to merge 7 commits into
d-v-b wants to merge 7 commits into
Conversation
Restricting a transform to a chunk box distributes over output dimensions whenever each output map reads its own input axis, which is every basic and orthogonal selection. Chunk resolution therefore no longer intersects the whole transform with every candidate chunk; it resolves each axis once against its grid into a table (StridedSet / IndexedSet), sorts correlated (vindex) index arrays into chunks once into a JointSet, and derives each ChunkProjection as one row of each table. ChunkPlan.partition() and partition_transform() expose the factored form, so a consumer can read the tables directly instead of materializing an object graph per chunk. The projections a plan yields are unchanged; the general whole-transform walk remains for hand-built diagonals, which have no factored form. Along the way: _intersect_general reuses a precomputed _CorrelatedBlock and accepts survivor positions; checked_affine has identity and dtype-bounded fast paths; ArrayMap._with_affine shares frozen index arrays on translate; IndexDomain._unchecked / IndexTransform._unchecked skip validation for objects derived from an already-valid transform. Assisted-by: ClaudeCode:claude-fable-5-1
Assisted-by: ClaudeCode:claude-fable-5-1
The package and its tests import nothing from zarr; the old comments claimed the chunk-resolution tests needed zarr's ChunkGrid, which stopped being true once the package grew its own grids. The real reason is the shared pinned test toolchain. Assisted-by: ClaudeCode:claude-fable-5-1
…hunk narrative The module docstring described intersecting the whole transform with every candidate chunk as "the algorithm"; that walk is now the fallback for hand-built diagonals only. It now explains the factored form and its three tables, and why they cost the sum of the touched chunks per axis. The visual guide gains a final integrator section, "A plan is a product of per-axis tables", with an executable snippet that reads the StridedSet, IndexedSet and JointSet tables off real plans and checks the plan's projections against the partition's rows. Integration boundaries gains "Reading the tables directly", a consumer that assembles a strided box from the tables with no projection materialized. The API index, landing page and design notes (TensorStore lineage, the performance caveat, and the box/query split) point at the new section. Assisted-by: ClaudeCode:claude-fable-5-1
…w fixes Adversarial review (roborev, a correctness reviewer, a complexity reviewer, and ~24k differential examples against main) of the grid partition. Cuts. The whole-transform walk that remained for hand-built diagonals is gone: it was unreachable for every index-array shape, its key builder was duplicated verbatim in _chunk_keys, and for the one shape it served it produced wrong projections (a three-point diagonal yielded four projections covering six cells, on main too). A DimensionMap diagonal is now rejected with ValueError. With it go the sorted-1-D fast path, the three cell-transform helpers, the block/positions parameters of _intersect_general, the correlated-residual check that admitted a diagonal and then crashed, GridPartition.__getitem__, partition_transform as public API, the object-dtype column fallback (StridedSet.origin is now a position along the request axis, so every column is intp), checked_affine's dtype-bound shortcut (measured at noise; the identity shortcut stays and now accepts bool via np.can_cast, as main did), and StridedSet.chunk_map/cell_map. Fixes. GridPartition.n_rows is an exact integer and len raises OverflowError instead of wrapping to zero; table columns are read-only, so a memoized partition cannot drift under a consumer; the documented table consumer now handles reversed axes, inserted axes and transposed transforms, and the snippet checks all three. Docs. Corrected the diagonal statement everywhere it appeared, the memoized "fresh walk" wording, the "vectorized per axis" claim, and the TensorStore correspondence (its strided sets are per input dimension; it keeps one index array set per connected component). The guide no longer restates the class docstrings. Assisted-by: ClaudeCode:claude-fable-5-1
A zero-stride DimensionMap over a domain wider than np.intp is valid and touches one storage cell; coercing every StridedSet column to intp made it raise OverflowError where main returned one projection. `extent` and `origin` are the two columns measured along the request axis, whose bounds are arbitrary Python ints, so they now fall back to exact-int (object) columns when a value does not fit. Chunk-local columns stay intp. Also corrects the design note that said both affine-diagonal cases raise NotImplementedError: two slice maps sharing an axis now raise ValueError. Assisted-by: ClaudeCode:claude-fable-5-1
7 tasks
…d the minimal grid protocol Every varying grid in the partition cases summed exactly to its extent, so the boundary where a chunk's data extent is shorter than its declared size, the rectilinear-specific case, was unpinned; so was a grid without data_size. Both now run through the evaluation oracle for strided, orthogonal and correlated selections. Assisted-by: ClaudeCode:claude-fable-5-1
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.
🤖 AI text below 🤖
Summary
Chunk resolution in
zarr-indexingbuilt twoIndexDomains, twoIndexTransforms and aChunkProjectionper chunk, and for correlated (vindex) selections rescanned the whole index array once per candidate chunk. This PR lifts that construction from the chunk to the axis.Restricting a transform to a chunk box distributes over output dimensions whenever each output map reads its own input axis (every basic and orthogonal selection). So each axis is resolved once against its grid into a table, and a projection is one row of each table:
StridedSet: one output dimension read through aConstantMaporDimensionMap, one row per touched chunk (chunk,chunk_start,chunk_extent,local_start,extent,origin,full).IndexedSet: an orthogonalArrayMap, its coordinates grouped by chunk in CSR form (pointer,index,positions, derivedlocal).JointSet: the correlated index arrays, sorted into chunks together once (chunk,pointer,index,positions,block_coordinates), since a chunk constrains all of them at once and they do not distribute.GridPartition: the tables plusrow_shape;n_rows,chunk_coords()(vectorized) and__iter__deriveChunkProjectionrows on demand. Columns are read-only.ChunkPlan.partition()memoizes the partition andChunkPlan.projections()iterates it; there is no second mechanism. A hand-built diagonal (twoDimensionMaps reading one input axis, which no selection produces) is rejected withValueError; the whole-transform walk that main used for it produced wrong projections (a three-point diagonal yielded four projections covering six cells), so rejection is strictly better. This is the shape of TensorStore'sIndexTransformGridPartition, with two simplifications noted in the design notes.The projections a plan yields are unchanged for every transform a selection can produce: a parametrized evaluation oracle (both transforms agree pointwise, cells tile the request exactly once, chunk-local coordinates lie in the chunk,
coverageisfullexactly when the chunk's cells are each read once) runs across basic, strided, reversed, scalar, orthogonal, one-element, correlated, 2-D block, sorted 1-D, empty and rank-0 cases on fixed and varying grids, and an out-of-tree differential harness matched main field-for-field over ~24k random transforms during review.Supporting changes:
_prepare_correlatedflattens a transform's correlated arrays once (shared by_intersect_generaland the joint table);checked_affinegains an identity fast path (measured to halve the correlated object-row walk; a dtype-bound variant measured at noise was dropped in review);ArrayMap._with_affineshares the frozen array when a map is translated;IndexDomain._unchecked/IndexTransform._uncheckedskip validation for objects derived from an already-valid transform (removing them measured +60% on basic and +130% on orthogonal walks).Reviewed adversarially before merge-readiness (roborev, a correctness reviewer, a complexity reviewer); the last commit applies their findings and cuts ~860 lines, including everything that made a second mechanism.
Performance
Indexer-level cost of planning plus consuming, measured through a
zarrindexer port on top of this package (legacy = today'szarr/core/indexing.py):"was" is
main. The object view is bounded by the five allocations per chunk it still makes; reading the tables is already faster than the legacy indexer, which is the form a consumer should move to. End-to-endzarrbenchmarks with the port went from 1.43x to 1.12x of legacy from the projection-path work alone; consuming the tables in the codec pipeline is the follow-up that would take that below 1.0x, and is not part of this PR.Documentation
The old narrative (intersect the whole transform with every candidate chunk) is retired everywhere it appeared, not just supplemented:
chunk_resolutionmodule docstring: explains the factored form, the three tables, the sum-versus-product cost, and that the whole-transform walk remains only for hand-built diagonals.docs/snippets/grid_partition.py, auto-discovered and run bytest_doc_examples.py) that readsStridedSet,IndexedSetandJointSettables off real plans and checks the plan's projections against the partition's rows.origindetail.IndexTransformGridPartition; the performance caveat; the box/query split) point at the new section.ChunkGridis corrected inpyproject.toml, thejustfile, and the CI workflow: nothing in the package importszarr; the suite runs from the repo root to share the pinned toolchain.mkdocs build --strictpasses.Testing
packages/zarr-indexing: 1331 passed, doctests and executable doc snippets included; pyright 0 errors; mypy belowmain's baseline; ruff clean;mkdocs build --strictpasses.tests/test_indexing.pyon unmodifiedzarr: 447 passed.🤖 Generated with Claude Code