BUG: Fix byteswapping for complex scalars #2886
Conversation
During a cleanup, the fast paths were invalidated because SIZEOF_LONGDOUBLE was not defined anymore and needs to be replaced with NPY_SIZEOF_LONGDOUBLE. The other SIZEOF macros still existed however so only complex long double broke because it switched to the already broken fast path. This commit fixes the fast path, and replaces all SIZEOF_ macros within arraytypes.c.src with their corresponding NPY_SIZEOF_ macros.
|
This change looks good to me. If there are still lots of places in the code that are using invalid macros, maybe we should just run a global copy/replace now while we're thinking about it? |
|
Looks good to me also. The unprefixed macro should be put into the noprefix.h header. You can probably look for other spots using a modification of the @certik These sort of fixes probably need a back port as 1.7 is supposed to be 'macro clean' and clearly there are some subtle problems that can appear when that isn't done. But I hate to suggest such intrusive changes after the first rc. |
BUG: Fix byteswapping for complex scalars
|
On a different note though, maybe someone can just tell me that this is actually plausible: In [7]: def tohex(num, swap=False):
return '-'.join(c.encode('hex') for c in (num if not swap else num.byteswap()).tostring()[::1 if swap else -1])
...:
In [8]: tohex(np.float128(1))
Out[8]: '83-00-01-83-00-00-3f-ff-80-00-00-00-00-00-00-00'
In [9]: tohex(np.float128(1))
Out[9]: '00-00-00-00-03-74-3f-ff-80-00-00-00-00-00-00-00'the underlying data is different, but even when double checking the value is identical also |
|
Intel long doubles are 96 bits padded to 128 bit alignment, so it's
|
|
This shows some more |
|
@seberg Do you want to make those fixes or should I? |
|
Please go ahead, this macro stuff is confusing me anyway... |
| # big-endian machine | ||
| assert_equal(x, np.fromstring(y.tostring(), dtype='<c8')) | ||
| """Ticket #1259 and gh-441""" | ||
| for dtype in [np.dtype('<'+t) for t in np.typecodes['Complex']]: |
There was a problem hiding this comment.
Well, t is one of:
In [2]: np.typecodes['Complex']
Out[2]: 'FDG'
but the original test was testing with t="c8" (effectively). So why was testing for "c8" removed?
There was a problem hiding this comment.
Its not removed np.dtype('F') == np.dtype('c8').
There was a problem hiding this comment.
Ah I see. All is ok then. Thanks!
I don't understand the paragraph. So |
|
@charris, the change looks ok to me, I only have one question above in the test ("c8"...) and then about the macros in my previous comment. After I understand the answers to those two questions, I'll see if we should backport it. |
|
I may have misinterpreted it (I don't know macro stuff), but my guess was, it is not defined anymore, but since it was only inside an |
|
@certik: Are you planning to do an rc2? |
|
@certik None of the uprefixed macros are defined for numpy sources, so any use of them is inviting a bug. Or may expose bugs ;) As Sebastian has pointed out, the compiler will not raise any errors if an undefined macro is used in a logic statement, it just evaluates to false. We will need to do another release candidate though. |
|
@njsmith, I was planning to just do the final release, but given this, I can do rc2. It's not a problem at all --- I have it all scripted now. I am now updating my Fabric fab files for Mac, so that they are fully automated too. So either is fine with me. |
|
Yeah, I guess it's a good idea to both backport those and do an rc2 then. |
|
OK. |
During a cleanup, the fast paths were invalidated because SIZEOF_LONGDOUBLE
was not defined anymore and needs to be replaced with NPY_SIZEOF_LONGDOUBLE.
The other SIZEOF macros still existed however so only complex long double
broke because it switched to the already broken fast path.
This commit fixes the fast path, and replaces all SIZEOF_ macros within
arraytypes.c.src with their corresponding NPY_SIZEOF_ macros.
This probably fixes issue gh-411 though that still needs checking. Note that there are still many occurrences where these macros need to be replaced. Also I noticed that the
noprefix.hdoes not includeSIZEOF_COMPLEX_*.