Feat/fast subslice reads#70
Merged
Merged
Conversation
…alize_dims Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…uested region Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
BrianWhitneyAI
marked this pull request as ready for review
June 18, 2026 16:25
BrianWhitneyAI
requested review from
BrianWhitneyAI,
SeanDuHare,
aswallace,
kmitcham and
pgarrison
June 18, 2026 16:25
Replace the Any typing on compute_dim_specs/reshape_data/_read_indexed with new DimSelection and DimSpec aliases, and parameterize the get_image_data and compute_dim_specs tests. finalize_dims test now verifies a real transpose. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
|
||
| This lets ``get_image_data`` read only the requested sub-region. | ||
| The default implementation materializes the full image and slices it; | ||
| readers that can read sub-regions cheaply should override it. |
Contributor
There was a problem hiding this comment.
Is this enough of a reference for readers to override?
Contributor
Author
There was a problem hiding this comment.
Here's how I would write the "This lets..." paragrah, starting over:
The default behavior here is to call self.data which can cause a delayed-load
of the whole image. Readers should override this function in order to do
more optimal finegrained data reads based on the `dim_specs`.
| sub-region. The default implementation materializes the full image and | ||
| slices it; readers that can read sub-regions cheaply should override it. | ||
|
|
||
| The result MUST equal ``self.data[tuple(dim_specs)]`` — i.e. integer specs |
Contributor
There was a problem hiding this comment.
Is this something we want in the docstring? my take was no
BrianWhitneyAI
approved these changes
Jun 18, 2026
BrianWhitneyAI
left a comment
BrianWhitneyAI
left a comment
Contributor
There was a problem hiding this comment.
Approved as Co-author, weigh review accordingly
SeanDuHare
approved these changes
Jun 23, 2026
| # All checks and operations passed, append dim operation to getitem ops | ||
| dim_specs.append(dim_spec) | ||
| # All checks and operations passed, append dim operation to getitem ops. | ||
| dim_specs.append(cast(types.DimSpec, dim_spec)) |
Contributor
There was a problem hiding this comment.
Might be nice to cast this earlier (maybe at kwargs.get(dim), line 180)
| def reshape_data( | ||
| data: types.ArrayLike, given_dims: str, return_dims: str, **kwargs: Any | ||
| ) -> types.ArrayLike: | ||
| def compute_dim_specs( |
Contributor
There was a problem hiding this comment.
Should this and finalize_dims be private (_compute_dim_specs) or are they intended to be used outside of reshape_data?
Contributor
There was a problem hiding this comment.
they're public because get_image_data calls them directly with _read_indexed in
between, so a subclass reader can build the indexer, read only that sub-region, then pad/transpose.
hughes036
approved these changes
Jun 23, 2026
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
4 participants
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
Previously
get_image_datacalledreshape_data(self.data, ...), which loaded the entire image before discarding everything outside the requested indices. This PR introduces a_read_indexedseam that receives the per-dimension getitem spec, so a reader that can read sub-regions cheaply can override it and skip the full read.What changed
reshape_datasplit into two halves (transforms.py), withreshape_dataretained as a thin wrapper so existing callers are unaffected:compute_dim_specs(shape, given_dims, return_dims, **kwargs)— the indexer-building half. Turns the kwargs selection into one getitem op per dimension plus the resulting dim order.finalize_dims(data, new_dims, given_dims, return_dims)— the reshape/transpose half. Pads missing return dims with depth-1 axes and transposes to the requested order._read_indexedseam (reader.py):Reader._read_indexed(given_dims, dim_specs) -> np.ndarray. The default implementation isself.data[tuple(dim_specs)](full read + slice), so behavior is unchanged for every existing reader.get_image_datanow runscompute_dim_specs→_read_indexed→finalize_dims.Typed dim selections/specs (
types.py):DimSelection = Union[int, List[int], Tuple[int, ...], range, slice]— a caller's per-dimension selection.DimSpec = Union[int, slice, List[int]]— the normalized getitem op produced bycompute_dim_specs.