BUG: Fix byteswapping for complex scalars #2886

Merged
merged 1 commit into from Jan 5, 2013

Conversation

Projects
None yet
4 participants
@seberg
Member

seberg commented Jan 5, 2013

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.h does not include SIZEOF_COMPLEX_*.

BUG: Fix byteswapping for complex scalars
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.
@njsmith

This comment has been minimized.

Show comment Hide comment
@njsmith

njsmith Jan 5, 2013

Member

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?

Member

njsmith commented Jan 5, 2013

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?

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Jan 5, 2013

Member

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 replace_old_macros.sed script in tools/ I run those scripts with find. There are lot of files to look at...

@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.

Member

charris commented Jan 5, 2013

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 replace_old_macros.sed script in tools/ I run those scripts with find. There are lot of files to look at...

@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.

charris added a commit that referenced this pull request Jan 5, 2013

Merge pull request #2886 from seberg/complex256-byteswap
BUG: Fix byteswapping for complex scalars

@charris charris merged commit 05dde0f into numpy:master Jan 5, 2013

1 check passed

default The Travis build passed
Details
@seberg

This comment has been minimized.

Show comment Hide comment
@seberg

seberg Jan 5, 2013

Member

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 == works, so I guess maybe my float128 is not a real float128? Of course the 3f-ff and some more seem to be always there.

Member

seberg commented Jan 5, 2013

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 == works, so I guess maybe my float128 is not a real float128? Of course the 3f-ff and some more seem to be always there.

@njsmith

This comment has been minimized.

Show comment Hide comment
@njsmith

njsmith Jan 5, 2013

Member

Intel long doubles are 96 bits padded to 128 bit alignment, so it's
possible that what you're seeing are differences in the contents of the
padding. (Not sure which bits are ignored.)
On 5 Jan 2013 15:43, "seberg" notifications@github.com wrote:

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 == works, so I guess maybe my float128 is not a real
float128? Of course the 3f-ff and some more seem to be always there.


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/2886#issuecomment-11915405.

Member

njsmith commented Jan 5, 2013

Intel long doubles are 96 bits padded to 128 bit alignment, so it's
possible that what you're seeing are differences in the contents of the
padding. (Not sure which bits are ignored.)
On 5 Jan 2013 15:43, "seberg" notifications@github.com wrote:

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 == works, so I guess maybe my float128 is not a real
float128? Of course the 3f-ff and some more seem to be always there.


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/2886#issuecomment-11915405.

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Jan 5, 2013

Member

This shows some more

charris@f16 [numpy.git (master)]$ grep -r '[^_]SIZEOF_' numpy/
Member

charris commented Jan 5, 2013

This shows some more

charris@f16 [numpy.git (master)]$ grep -r '[^_]SIZEOF_' numpy/
@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Jan 5, 2013

Member

@seberg Do you want to make those fixes or should I?

Member

charris commented Jan 5, 2013

@seberg Do you want to make those fixes or should I?

@seberg

This comment has been minimized.

Show comment Hide comment
@seberg

seberg Jan 5, 2013

Member

Please go ahead, this macro stuff is confusing me anyway...

Member

seberg commented Jan 5, 2013

Please go ahead, this macro stuff is confusing me anyway...

@seberg seberg deleted the seberg:complex256-byteswap branch Jan 5, 2013

- # 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']]:

This comment has been minimized.

Show comment Hide comment
@certik

certik Jan 5, 2013

Contributor

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?

@certik

certik Jan 5, 2013

Contributor

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?

This comment has been minimized.

Show comment Hide comment
@seberg

seberg Jan 5, 2013

Member

Its not removed np.dtype('F') == np.dtype('c8').

@seberg

seberg Jan 5, 2013

Member

Its not removed np.dtype('F') == np.dtype('c8').

This comment has been minimized.

Show comment Hide comment
@certik

certik Jan 5, 2013

Contributor

Ah I see. All is ok then. Thanks!

@certik

certik Jan 5, 2013

Contributor

Ah I see. All is ok then. Thanks!

- 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']]:
+ z = np.array([2.2-1.1j], dtype)

This comment has been minimized.

Show comment Hide comment
@certik

certik Jan 5, 2013

Contributor

So you changed -1j to 2.2-1.1j, which seems ok to me.

@certik

certik Jan 5, 2013

Contributor

So you changed -1j to 2.2-1.1j, which seems ok to me.

+ assert_equal(x, np.fromstring(y.tostring(), dtype=dtype))
+ # double check real and imaginary parts:
+ assert_equal(x.real, y.real.byteswap())
+ assert_equal(x.imag, y.imag.byteswap())

This comment has been minimized.

Show comment Hide comment
@certik

certik Jan 5, 2013

Contributor

The rest of the changes here look ok to me.

@certik

certik Jan 5, 2013

Contributor

The rest of the changes here look ok to me.

@certik

This comment has been minimized.

Show comment Hide comment
@certik

certik Jan 5, 2013

Contributor

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.

I don't understand the paragraph. So SIZEOF_LONGDOUBLE is still defined, and that's the reason why the old code compiled, correct? What is the difference between NPY_SIZEOF_LONGDOUBLE and SIZEOF_LONGDOUBLE?

Contributor

certik commented Jan 5, 2013

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.

I don't understand the paragraph. So SIZEOF_LONGDOUBLE is still defined, and that's the reason why the old code compiled, correct? What is the difference between NPY_SIZEOF_LONGDOUBLE and SIZEOF_LONGDOUBLE?

@certik

This comment has been minimized.

Show comment Hide comment
@certik

certik Jan 5, 2013

Contributor

@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.

Contributor

certik commented Jan 5, 2013

@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.

@seberg

This comment has been minimized.

Show comment Hide comment
@seberg

seberg Jan 5, 2013

Member

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 #if ... preprocessor statement that statement that #if simply evaluated false dropping through to the general not unrolled for loop case (which was buggy prior to this, but since no compiler has a floating with more then 16 bytes it never mattered).

Member

seberg commented Jan 5, 2013

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 #if ... preprocessor statement that statement that #if simply evaluated false dropping through to the general not unrolled for loop case (which was buggy prior to this, but since no compiler has a floating with more then 16 bytes it never mattered).

@certik

This comment has been minimized.

Show comment Hide comment
@certik

certik Jan 5, 2013

Contributor

Ah, I see how it works. Yes, in this case I think we should backport it. @charris, @njsmith, let me know if you agree.

Contributor

certik commented Jan 5, 2013

Ah, I see how it works. Yes, in this case I think we should backport it. @charris, @njsmith, let me know if you agree.

@njsmith

This comment has been minimized.

Show comment Hide comment
@njsmith

njsmith Jan 5, 2013

Member

@certik: Are you planning to do an rc2?

Member

njsmith commented Jan 5, 2013

@certik: Are you planning to do an rc2?

@charris

This comment has been minimized.

Show comment Hide comment
@charris

charris Jan 5, 2013

Member

@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.

Member

charris commented Jan 5, 2013

@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.

@certik

This comment has been minimized.

Show comment Hide comment
@certik

certik Jan 5, 2013

Contributor

@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.

Contributor

certik commented Jan 5, 2013

@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.

@certik certik referenced this pull request Jan 5, 2013

Closed

Backport reminder #2889

@njsmith

This comment has been minimized.

Show comment Hide comment
@njsmith

njsmith Jan 6, 2013

Member

Yeah, I guess it's a good idea to both backport those and do an rc2 then.

Member

njsmith commented Jan 6, 2013

Yeah, I guess it's a good idea to both backport those and do an rc2 then.

@certik

This comment has been minimized.

Show comment Hide comment
@certik

certik Jan 6, 2013

Contributor

OK.

Contributor

certik commented Jan 6, 2013

OK.

@certik certik referenced this pull request Jan 10, 2013

Merged

Backport2886and7 #2900

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment