MAINT: gracefully shuffle memoryviews#18282
Conversation
* allow graceful shuffling of memoryviews, with same behavior as arrays, instead of producing a warning on `memoryview` shuffle
numpy/random/_generator.pyx
Outdated
| char* x_ptr | ||
| char* buf_ptr | ||
|
|
||
| if isinstance(x, memoryview): |
There was a problem hiding this comment.
I'm guessing this conditional could just be removed? --was being conservative I suppose
There was a problem hiding this comment.
Since x can be a list that we definitely do not want to view as an ndarray, this conditional is not optional.
|
I wonder if we should instead change the |
That was specific to that particular use of memoryviews post-shuffle, not the shuffling per se. This is a good change. |
|
Both changes seem fine to me ( I am not quite sure that the memoryview change can't go wrong in principle (memoryviews do support suboffsets for example), but I am willing to take the risk of such an unlikely backcompat break (it would go to an error), it probably is nicer for memoryviews. Although, I doubt there is a reason to optimize for them here. |
* based on reviewer feedback, instead of explicitly supporting `memoryview` shuffling via `asarray()`, support the shuffling implicitly by using `Sequence` instead of `MutableSequence`
|
I pushed in Eric's suggestion after confirming the random suite still passes locally for this feature branch. |
|
Thanks, LGTM. I find it a bit surprising that a memoryview isn't considered a mutable sequence (is that supposed to be only the length?), but I guess if Python changes it, we have a test that notifies us. I would be fine with a memoryview fast path as you had it, although I guess this might be nicer for backporting. |
|
|
|
Ah right. So I think this is still good enough to warn for tensorflow as well? In which case I think we should go with it or is there some other reason to prefer MutableSequence, that I am missing? Although, I would be happy to entertain the thought of making things more strict in 1.21. |
|
Thanks Tyler. |
Fixes gh-18273
same behavior as arrays, instead of producing
a warning on
memoryviewshuffleThere's certainly some contention that me doing this downstream was sub-optimal anyway, but I don't think there's any claim that it should be disallowed/considered a bug to shuffle a memoryview.
I was originally going to special case 1-D C-contiguous memoryviews, but the discussion in the matching issue seems to suggest that just using
asarray()without fancy special casing might be the way to go?