BUG: Fix unicode with byte swap transfer and copyswap #7664
Conversation
| @@ -1086,7 +1086,8 @@ _strings_richcompare(PyArrayObject *self, PyArrayObject *other, int cmp_op, | |||
| int val; | |||
|
|
|||
| /* Cast arrays to a common type */ | |||
There was a problem hiding this comment.
That whole casting if, could probably be moved to the new iterator, but I think that is good enough on another day maybe.
|
OK, added some basic tests. To be honest, I guess nobody ever runs into this. I suppose nobody saves unicode arrays on big endian machines and tries to load them on another one ;). And all the other libs just use string/byte arrays and decode those. |
| int i; | ||
|
|
||
| while (N > 0) { | ||
| memcpy(dst, src, dst_itemsize); |
There was a problem hiding this comment.
Doesn't this read too far in src if src_itemsize < dst_itemsize?
There was a problem hiding this comment.
Oh yeah, good point. I guess the tests just don't care that much about a bit reading too far ;). (I hope valgrind would show it but OK).
Just to note, I did not check the python 3 failure here, and I noticed there is a bug that casting from S -> nonnative U actually seems to reverse the string, and I am not sure why.
There was a problem hiding this comment.
OK, checked, the tests show the bad behaviour in valgrind, so I will call it covered by tests already considering that it is non-trivial to test more reliably then that.
There was a problem hiding this comment.
Fixed both things, but the extra inversion of the fields really throws me off still
There was a problem hiding this comment.
OK, fixed that weird string cast as well, heh.
seberg
force-pushed
the
fix-unicode-trans
branch
4 times, most recently
from
b60965c
to
414bca2
Compare
| @@ -3628,11 +3672,19 @@ PyArray_GetDTypeTransferFunction(int aligned, | |||
| } | |||
| } | |||
|
|
|||
| /* The special types, which have no byte-order */ | |||
| /* The special types, which have no or subelement byte-order */ | |||
| switch (src_type_num) { | |||
| case NPY_VOID: | |||
There was a problem hiding this comment.
while NPY_VOID will always fail the if-statement in the next NPY_UNICODE case (there's no bug), maybe it is clearer to put the NPY_VOID case below the unicode case?
|
It looks good to me so far. I just want to check one or two things, I'll get back to it soon. |
| else if (dtype->kind == 'U') { | ||
| return wrap_copy_swap_function(aligned, | ||
| src_stride, dst_stride, dtype, 1, | ||
| outstransfer, outtransferdata); |
There was a problem hiding this comment.
Might this be changed to a call to PyArray_GetStridedZeroPadCopyFn? I would imagine that would be a small performance gain over using the element-wise copy_swap function.
There was a problem hiding this comment.
Sure, can change, not sure it should be faster, since I would expect this uses the copyswapn function and not copyswap which might actually be quicker, but did not check.
|
OK, finished reading. Everything else looks good to me. |
|
@seberg, good point that it uses copyswapn instead of copyswap. Probably it is pretty fast as-is. In that case, is there anything else you want to add? If not, it looks good to me and we can merge. |
|
Could still squash (on the run can do tomorrow). Don't have anything to add, can't think of any big bug surrounding this I might have missed. If you have any nitpick will fix of course. |
These fixes should make unicode byteswapping not be completely broken. The code is not so much designed for absolute speed. Fixes numpygh-3939
Casting to non-negative unicode used the wrong swapping functionality. Just to note, the whole swapping is rather inefficient, since it is completely unnecessary due to the fact that we go via python in any case, though likely it does not matter.
|
OK, squased/rebased from bus, so got to let the test run through (will probably take a while). |
|
Tests passed, proving bus-based development works. Merging now. Thanks Sebastian! |
|
Thanks for the review Allan! |
These fixes should make unicode byteswapping not be
completely broken. The code is not so much designed for absolute
speed.
Tests still needed any comments appreciated. I tried to keep things simple, so used the copyswap function wrappers....