Always skip reads when completely overwriting chunks #2784
+138
−83
Conversation
This file contains 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
github-actions
bot
added
the
needs release notes
d-v-b
reviewed
Jan 31, 2025
d-v-b
reviewed
Jan 31, 2025
d-v-b
reviewed
Jan 31, 2025
d-v-b
reviewed
Jan 31, 2025
Uses `slice(..., None)` to indicate that a `chunk_selection` ends at the boundary of the current chunk. Also does so for a last chunk that is shorter than the chunk size. `is_total_slice` now understands this convention, and correctly detects boundary chunks as total slices. Closes zarr-developers#757
This reverts commit 234431cd6efb661c53e2a832a0e4ea4dca772c1b.
…nks" This reverts commit edbba37.
dcherian
force-pushed
the
optimize-boundary-chunk-writes
branch
from
1da36f4 to
0e00eb3
Compare
d-v-b
reviewed
Feb 5, 2025
dcherian
commented
Feb 5, 2025
| @@ -1198,7 +1206,8 @@ def __iter__(self) -> Iterator[ChunkProjection]: | |||
| for (dim_sel, dim_chunk_offset) in zip(self.selection, chunk_offsets, strict=True) | |||
| ) | |||
|
|
|||
| yield ChunkProjection(chunk_coords, chunk_selection, out_selection) | |||
| is_complete_chunk = False # TODO | |||
There was a problem hiding this comment.
I'm not sure where this is used. Is it always True?
github-actions
bot
removed
the
needs release notes
|
#2784 fixes the |
dcherian
force-pushed
the
optimize-boundary-chunk-writes
branch
from
1921878 to
b2be002
Compare
dcherian
changed the title
|
I find the indexing code pretty hard to follow, but this looks good to me |
d-v-b
approved these changes
Feb 5, 2025
|
@normanrz - do you have the bandwidth to give this a review? |
|
@normanrz if you have time, please prioritize reviewing zarr-developers/numcodecs#700 over this one. |
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.
Teach the *Indexer objects to detect when a selection corresponds to a whole chunk. Propagate that information down to the codec pipeline where reads can be skipped if it makes sense locally.
Closes #757
I'm opening this for comments about the approach (@d-v-b , @normanrz ) it could definitely use more tests (edit: and some fixes)TODO:
changes/