Changeset 8438 breaks ldexp on 64bit/bigendian systems (Trac #1633) #2229

numpy-gitbot · 2012-10-19T22:18:37Z

Original ticket http://projects.scipy.org/numpy/ticket/1633 on 2010-10-11 by @nstange, assigned to unknown.

Testsuite fails with:

FAIL: test_ldexp (test_umath.TestLdexp)

Traceback (most recent call last):
File "/pf/m/m222086/xas/solaris10/64/python2/python-2.7-ve0-gcc/lib/python2.7/site-packages/numpy/core/tests/test_umath.py", line 391, in test_ldexp
assert_almost_equal(ncu.ldexp(2., 3), 16.)
File "/pf/m/m222086/xas/solaris10/64/python2/python-2.7-ve0-gcc/lib/python2.7/site-packages/numpy/testing/utils.py", line 463, in assert_almost_equal
raise AssertionError(msg)
AssertionError:
Arrays are not almost equal
ACTUAL: 2.0
DESIRED: 16.0

raise AssertionError('\nArrays are not almost equal\n ACTUAL: 2.0\n DESIRED: 16.0')

The issue has been introduced here:
http://projects.scipy.org/numpy/changeset/8438

The reason is in DOUBLE_ldexp (loops.c.src:1122):
const int in2 = *(int *)ip2;

Since ip2 is being cast from (long_) to (int_), and thus in2 (exponent) will only receive the first 4 bytes, that is the 4 most significant bytes on bigendian, that is zero on 64bit.

System's ldexp's exp-argument is always an int, and thus in my opinion, the correct behaviour is to reject any exp-argument we cannot convert to an system's int, that is an np.intc, and adjusting the docs and the testsuite rather than silently giving invalid results.

The attached diff

fixes the ldexp_signatures to match the system's ldexp again (broken by changeset 8438)
adjusts the testsuite to only put in system's ints

numpy-gitbot · 2012-10-23T02:44:52Z

Attachment added by @nstange on 2010-10-11: 06_fix_ldexp_signature.diff

numpy-gitbot · 2012-10-23T02:44:52Z

@charris wrote on 2010-10-11

This looks like my goof in making the commit. I think you are correct here.

numpy-gitbot · 2012-10-23T02:44:52Z

@pv wrote on 2010-10-11

I think this patch is problematic -- it seems it will reintroduce #1464.

Not accepting the platform default int type (which is long) is asking for trouble. I don't see really good ways around this. I'd perhaps just check the size of the integer, and emulate the HUGE_VAL return that ldexp normally does.

numpy-gitbot · 2012-10-23T02:44:52Z

@pv wrote on 2010-10-11

"platform default int" -> "Python default int", of course.

numpy-gitbot · 2012-10-23T02:44:53Z

@nstange wrote on 2010-10-11

The problem of #1464 is not with ldexp, but with the fact that numpy cannot convert np.intc to np.int.
This is the real issue and in #1464 it errors out because it cannot fulfill the ldexp-request with the input data it got. numpy is completely right about that: It can not. The np.int-array should be cast to an np.intc array before, either manually or automatically by numpy.

In the meanwhile: If you ask me, getting wrong results is more serious to scientists than getting error messages. My personal opinion is that the attached diff is less problematic than #1464. On the other side it might break existing python code...

Just to be curious: Are casts implemented to the bigger types, that is float->double, intc->int?

numpy-gitbot · 2012-10-23T02:44:53Z

@pv wrote on 2010-10-11

I'm not saying that it should be left as it is now -- I'm just saying that ldexp(.4,5) should also work. How it worked before #1464 is indeed more correct, but I believe we can get both correct results and no error messages :)

Generally, casts to "bigger" types are automatically done. You can see what via

>>> np.can_cast(np.single, np.double)
True
>>> np.can_cast(np.double, np.single)
False

numpy-gitbot · 2012-10-23T02:44:53Z

@nstange wrote on 2010-10-11

I'm not saying that it should be left as it is now
Sorry, i didn't want to be crude. It just sounded to be as if you wanted to change the ldexp_signatures back to using PyArray_LONG and introducing some crazy pointer magic ;)

Why is using numpy a long as default, not an int? There are reasons for why the int's are what they are on every platform, I guess (efficiency, storage size, application of SIMDs, ...).
But now, being faced with the reality: What about introducing a cast from np.int to np.intc that possibly throws on overflow?

numpy-gitbot · 2012-10-23T02:44:53Z

@pv wrote on 2010-10-11

Suggested fix: http://github.com/pv/numpy/compare/master...bug/1633-ldexp-long.diff

I don't have big-endian machines at hand, so testing is appreciated (although it should work). Thanks!

***

The easiest solution here seemed to be to extend the ldexp definition to the whole long range -- in this case we're lucky and it is easy to do.

I don't actually know what would be the easiest way to force downcast + raise overflow error. This is not the only place where this int vs. long annoyance has come up. Seems like this could be a useful ufunc feature more generally.

***

I think the reason why Numpy's default integer type is long is that the Python integer type is also long. So we probably inherited the default from there.

numpy-gitbot · 2012-10-23T02:44:54Z

@nstange wrote on 2010-10-12

I don't have big-endian machines at hand, so testing is appreciated (although it should work). It works superb! Thank you!

Just one little note:
Your new testcase asserts a warning:

ncu.ldexp(2., imax)
Warning: overflow encountered in ldexp
inf

I know that this is correct, but the other testcases don't output anything to stdout/err...

I don't actually know what would be the easiest way to force downcast + raise overflow error. This is not the only place where this int vs. long annoyance has come up. Seems like this could be a useful ufunc feature more generally.
Grep is my friend :) Have a look at arraytypes.c.src.
LONG_to_INT is already in there, but it is autogenerated and doesn't check for overflows.
At the moment, this conversion is not allowed, see the whitelist of allowed conversions in PyArray_CanCastSafely (the long switch statement at the end, cancastto==NULL for the builtins).

I think the way to go here is to make the conversion functions in arraytypes.c check for overflows and possibly throw an 8000 d allow that conversion in PyArray_CanCastSafely. But since this is a major design decision (for implicit conversions users would possibly encounter OverflowErrors they don't understand), it doesn't belong into this bug report, I guess.
At the moment your diff fixes the issue very well.

I think the reason why Numpy's default integer type is long is that the Python integer type is also long. So we probably inherited the default from there.
Good point

numpy-gitbot · 2012-10-23T02:44:54Z

@pv wrote on 2010-10-16

b55eacd

93f7521

numpy-gitbot · 2012-10-23T02:44:54Z

@rgommers wrote on 2010-10-17

Still an issue on Windows XP 32-bit (not on OS X). With py25:

======================================================================
FAIL: test_ldexp_overflow (test_umath.TestLdexp)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python25\Lib\site-packages\numpy\core\tests\test_umath.py", line 411, in test_ldexp_overflow
    assert_equal(ncu.ldexp(2., imax), np.inf)
  File "C:\Python25\Lib\site-packages\numpy\testing\utils.py", line 303, in assert_equal
    raise AssertionError(msg)
AssertionError: 
Items are not equal:
 ACTUAL: 0.0
 DESIRED: 1.#INF

And with py31:

.......Warning: overflow encountered in ldexp

This with current 1.5.x branch.

numpy-gitbot · 2012-10-23T02:44:54Z

@rgommers wrote on 2010-10-17

Also only a warning printed with 2.6 and 2.7 on Windows. So only 2.5 fails (and 2.4 probably, can't test that now).

numpy-gitbot · 2012-10-23T02:44:55Z

numpy-gitbot commented

Oct 23, 2012

@pv wrote on 2010-10-18

I can reproduce it on Wine w/ Python 2.5 (Python 2.4 distutils doesn't seem to work with numpy distutils on Windows any more). Damnation this combinatorial explosion in platforms and Python versions.

numpy-gitbot · 2012-10-23T02:44:55Z

@rgommers wrote on 2010-10-19

Actually, under Wine is how I tested it. Sorry for not mentioning that all the time, but I've never found a single numpy/scipy issue where Wine and XP were different. When I do, I'll start being more consistent.

Testing on Windows is usually done with binaries built with Wine on OS X anyway.....

numpy-gitbot · 2012-10-23T02:44:55Z

@pv wrote on 2010-10-19

Something very strange is going on here:

>>> import numpy as np
>>> np.iinfo('l').max
2147483647
>>> np.ldexp(2, 2147483647)
0.0
>>> np.ldexp(2, 2147483646)
0.0
>>> np.ldexp(2, 2147483645)
Warning: overflow encountered in ldexp
inf

There seems to be exactly two integer values for which it does not work. And this depends on the Python version. I will need to set up a debugger on a real Windows platform to figure it out (I haven't managed to get debuggers work Wine to work, or Numpy build with debugging symbols to succeed). This may take some time.

Moreover, the new code path is not invoked here, since on this setup SIZEOF_INT == SIZEOF_LONG. Indeed, I can reproduce the issue on the preceding d24db3430a, so this bug is unrelated to #2229.

numpy-gitbot · 2012-10-23T02:44:55Z

@pv wrote on 2010-10-19

Continued in #2237

numpy-gitbot closed this as completed Oct 19, 2012

numpy-gitbot mentioned this issue Oct 23, 2012

Strange behavior of ldexp on Python2.5 + Wine (Trac #1641) #2237

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Changeset 8438 breaks ldexp on 64bit/bigendian systems (Trac #1633) #2229

Changeset 8438 breaks ldexp on 64bit/bigendian systems (Trac #1633) #2229

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Changeset 8438 breaks ldexp on 64bit/bigendian systems (Trac #1633) #2229

Changeset 8438 breaks ldexp on 64bit/bigendian systems (Trac #1633) #2229

Comments

Testsuite fails with:

FAIL: test_ldexp (test_umath.TestLdexp)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!