Support negative step in normalize_indexer#11044
Conversation
keewis
changed the title
|
The failure seems to be unrelated Edited: After testing more carefully I can confirm that the patch actually fixes #11000. |
jsignell
left a comment
jsignell
left a comment
There was a problem hiding this comment.
This looks good @avalentino sorry it took a while for anyone to review. It slipped through the cracks around the holidays. Do you want to add a section to what's new?
jsignell
left a comment
jsignell
left a comment
There was a problem hiding this comment.
Actually some of the hypothesis tests are failing: https://github.com/pydata/xarray/actions/runs/21528473706/job/62038163305?pr=11044#step:10:375
|
Hi @jsignell , should be fixed now. |
| >>> _expand_slice(slice(0, -1), 10) | ||
| array([0, 1, 2, 3, 4, 5, 6, 7, 8]) | ||
| """ | ||
| sl = normalize_slice(slice_, size) |
There was a problem hiding this comment.
why aren't we using normalize_slice anymore?
There was a problem hiding this comment.
Basically this is just using the old version of the function. To me it looks like the alternative would be to catch stop=None and set it to -1 before passing it into np.arange. But slice(*slice_.indices(size)) already returns stop=-1 anyways so 🤷
There was a problem hiding this comment.
I think we have two options: add an option to normalize_slice that allows switching between targeting range or slice (we might need to rename the function then), or just use slice.indices directly. The latter would mean that we'd do this here:
start, stop, step = slice_.indices(size)
return np.arange(start, stop, step)There was a problem hiding this comment.
I have implemented what @keewis suggestd
jsignell
left a comment
jsignell
left a comment
There was a problem hiding this comment.
This looks good to me. Just a suggestion about the docstring.
Co-authored-by: Julia Signell <jsignell@gmail.com>
|
Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again! |
4 participants
whats-new.rstapi.rst