Detect out-of-range integer indices in __setitem__ operations
#378
Conversation
|
(Resolved a trivial changelog conflict.) |
davidhassell
left a comment
davidhassell
left a comment
There was a problem hiding this comment.
Nicely crafted. A bit of logic needs adding for -ve integers, and as a result the tests extended, but that's all. I think we should marks it with the NEXTVERSION milestone :)
|
Thanks for your prompt review @davidhassell. I believe I have addressed all feedback, except:
I am not sure where you mean to put that - I can't think of a good place in this context? |
It's on the RHS, a bit below where we set "label" |
Co-authored-by: David Hassell <[email protected]>
Oh 🤦🏻♀️ I now realise you mean the PR! I thought you were meaning on the code as a |
| ) | ||
|
|
||
| if keepdims: | ||
| # Convert an integral index to a slice |
There was a problem hiding this comment.
I think we need some indentation here?
There was a problem hiding this comment.
(my fault for not including it in my last suggestion!)
There was a problem hiding this comment.
AH - I made a comment about this on the thread and now I don't see it. Not sure why it's disappeared!
There was a problem hiding this comment.
OK after checking my tabs said comment is gone with no trace - did I just imagine/dream posting it?
What I said was (in said dream, or otherwise) is that there is an elif hasattr(index, "to_dask_array"): following the original if keepdims and isinstance(index, Integral): which means that converting to having the top-level if isinstance(index, Integral): actually complicates the flow - we'd need further somewhat contrived checking afterwards to get the equivalent. So actually I am not sure it's a good idea.
That said, no need to actually call isinstance twice - so I suggest we store that in a variable to use each time but use the original conditional structure. What do you think?
There was a problem hiding this comment.
GitHub having a moment ...
Thanks for spotting this. I think what you say is worthwhile. It may seem like overkill (how long does an isinstance really take?), but this is library code, and parse_indices gets call called a lot.
There was a problem hiding this comment.
No not overkill, you made a very good point about the duplication - but I should effectively revert the original suggestion to keep the conditionals simple given that elif, whilst getting rid of the second isinstance call. Will do that now if you're happy...
2 participants
Close #377. Also added an equivalent test for
__getitem__which passed to begin with, for completeness.