-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
BUG: Don't fail when lexsorting some empty arrays. #14240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good, also nice additional error path cleanups. I noticed that there are some npy_free_cache
calls below. I think it would actually make sense to endorse that and use npy_alloc_cache
, but mixing the two smells like an error.
I also thought this was a bit suspicious. I've switched them to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine, just small things to fix up. I am still a bit torn about the special case, it might be that the sorting code is not safe for 1 (or even 0) elements in some cases, although this function likely is. If we can just remove the whole part that would be nicer, since it is not speed relevant for sure.
assert xs.strides == (8,) | ||
assert np.lexsort((xs,)).shape[0] == 0 # Works | ||
|
||
xs.strides = (16,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for now, so I will not press to change it. I do not like the stride manipulation as such though. What would be safest may be to use xs = np.array([1, 2, 3], dtype='i8')
and then multiple slicing as in xs[:0]
and xs[::2][:0]
(you can assert the strides after that).
Or maybe, just for the x.strides = ...
part, do x = xs[::2]
instead (it should work) and just assert in case the behaviour ever changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's wrong with changing the strides like this? Numpy isn't going to change semantics such that this would break because it would break too much user code.
I think this way of doing it is easier to read than your suggestion because you can literally read off what the strides of the new xs
will be, which is important information for reasoning about how the lexsort function will execute. With your suggestion the strides
, which are what you really care about, are only updated in an indirect way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, yes, we should, and eventually probably will deprecate such usage, because that usage is broken in many ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that an empty array should rightfully be able to have any strides at all set on it, but maybe this breaks something elsewhere in numpy?
Anyway, I don't really fancy changing this if you aren't going to require it, because I do think it is the clearest presentation of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the other approach is to use np.lib.stride_tricks.as_strided
, but OK. Whenever we do the deprecation we can just fixup the test. Of course it is safe for empty arrays or arrays with a single element, but that is about the only case where it is sane.
a248393
to
c72454a
Compare
@seberg Happy with this? |
Added a commit to prefer |
5c42b99
to
00069b0
Compare
The Shippable failure looks spurious, restarted. |
OK, added a guard on I would just merge it as is. Seems the shippable failed again? |
close/reopen. |
Close/reopen to retrigger shippable. I think the error is spurious, we had a streak of the same a while back, but it is nice when everything passes. |
OK, let me put this in. Thanks @batterseapower. |
np.lexsort
would fail on empty arrays with non-standard strides as a result of the changes in #13691. While fixing this I tried to also solve a couple of other problems/leaks that could be triggered in low-memory situations.Fixes #14228