FEAT/FIX: more consistent move behavior + options on reindex#1621
FEAT/FIX: more consistent move behavior + options on reindex#1621
Conversation
|
I still have the stateful tests to fill out with this new behavior (and i need to look more careful at the claude generated unit tests) but I wanted to open now to get a review on the behavior changes before I put that effort in. I know @dcherian was. committed to reviewing this, but for the behavior @paraseba would be great to hear from you. |
|
after talking wiht @dcherian :
test reindiex by using hypothesis property test (equivalent to vindex with a random array) using permutation |
paraseba
left a comment
paraseba
left a comment
There was a problem hiding this comment.
I like what Deepak said too. Didn't look at the code in detail yet
|
|
||
| Returns | ||
| ------- | ||
| tuple[int, ...] |
There was a problem hiding this comment.
Like this idea for the return type. The documentation may need an example.
| array_path: str, | ||
| chunk_offset: Iterable[int], | ||
| ) -> tuple[int, ...]: | ||
| """Roll (circular shift) all chunks in an array by the given chunk offset. |
There was a problem hiding this comment.
Is this a common use case? Can you give me examples?
There was a problem hiding this comment.
Admittedly I can't think of one, but that would've also been true for shift-array. It seems to me like the other natural move operation that users will eventually want, so nice to not ask them to re-implement. its also the behavior I would've expected naively.
|
Reminder that I stole the |
|
@paraseba is there anything more here? I think i addressed all of deepak's feedback |
ianhi
force-pushed
the
ian/fix/shift-array-modes
branch
from
896c5da to
ba9ddae
Compare
3 participants
This PR makes (to my eye) the behavior of shift_array and reindex more consistent and explicit about user intent.
Wrapping and discard
Prior:
asymmetric behavior of chunk deletion vs error for negative and positive shfits: #1619
now: User must opt either to
wrapmode ordiscardmodereindex stale chunks vs deleting
Prior: reindex would always leave stale chunks if a chunk was moved but not replaced.
Now: user must chose between deleting vacated chunks or leaving them
shift_array return
since shift array works in chunk space it might be convenient for the user get back the shifts in array index space. I added a return to shift array for this.
Docs
move, shift_array, and reindex have docs pages now!
shift/reindex: https://icechunk--1621.org.readthedocs.build/en/1621/moving-chunks/
move: https://icechunk--1621.org.readthedocs.build/en/1621/moving-nodes/