BUG: Fix casting from longdouble to long #9971

eric-wieser · 2017-11-06T05:22:59Z

Implements a custom PyLong_FromLongDouble derived from PyLong_FromDouble.

Depends on #9967 being merged, but submitting now to allow tests to run on a platform with longdouble support, something mine does not have. I'll rebase once that's merged.

charris · 2017-11-06T20:37:49Z

#9967 is in and needs rebase.

ahaldane · 2017-11-06T22:51:17Z

numpy/core/src/private/npy_longdouble.c

+    /* Get the MSBs of the integral part of the float */
+    frac = npy_ldexpl(frac, (expo-1) % chunk_size + 1);
+    for (i = ndig; --i >= 0; ) {
+        npy_int32 chunk = (npy_int32)frac;


Why not npy_uint64 with chunk_size 64?

digits in the original code was unsigned

Hadn't noticed that it was unsigned. Yes, could probably change to sizeof(npy_long) * CHAR_BIT and npy_long

Changed to use PyLong_FromUnsignedLongLong, npy_ulonglong, and NPY_BITSOF_ULONGLONG

ahaldane · 2017-11-06T22:53:19Z

numpy/core/src/private/npy_longdouble.c

+        if (v == NULL) {
+            goto done;
+        }
+        l_chunk = PyLong_FromLong(chunk);


OK, maybe the reason for 32 bit chunk is because npy_long can be 32bit, and chunk needs to be npy_long.

ahaldane · 2017-11-06T23:30:36Z

numpy/core/src/private/npy_longdouble.c

+done:
+    Py_DECREF(l_chunk_size);
+    return v;
+}


There is a funny character here. ~~Windows newline?~~ OK, it says "No newline at end of file", so no problem.

Newline added

eric-wieser · 2017-11-07T03:25:24Z

numpy/core/src/private/npy_longdouble.c

+ * Notably, we can't set the digits directly, so have to shift and or instead.
+ */
+PyObject *
+PyLong_FromLongDouble(npy_longdouble ldval)


Alternate name proposal npy_longdouble_to_PyLong.

Either seems fine. Maybe the npy_ alternative is safer in case Python3 decides to define this too.

eric-wieser · 2017-11-07T03:37:41Z

numpy/core/src/umath/scalarmath.c.src

-#if @realtyp@
-    double ix;
-    modf(x, &ix);
-    x = ix;


This seemed to be just for the long/int choice below, which is far easier (although a little slower) to handle by just producing a long, and letting python decide whether to downcast to an int.

I think we can now remove the @realtype@ repeat var. @unsigntyp@ too

(I don't exactly understand why these lines were needed before, since C-casting truncates the double fractional part anyway.. but the new code looks correct so I won't worry too much).

Oh I think it was probably so the x < LONG_MAX and x < LONG_MIN checks aren't affected by the fractional value, in the case of exact equality of the integer part of x to those values.

Indeed, that was the purpose - but I don't think the decrease in clarity is worth it for a marginal performance improvement on only python 2.

I'll remove the repeat vars - good catch

eric-wieser · 2017-11-07T03:43:04Z

numpy/core/src/private/npy_longdouble.c

+        PyObject *_py_tmp = (PyObject *)(op);   \
+        (op) = (op2);                           \
+        Py_DECREF(_py_tmp);                     \
+    } while (0)


This would probably be generally handy throughout the codebase.

A job for another PR: would there be a better place to put it, and do we want to cal lit Py_SETREF and #ifndef Py_SETREF it?

If we decide to use it I agree that doing and #ifndef seems reasonable.

For anyone else reading, it looks like this python macro was created in python3.4, and there are a lot of discussions of it on the python mailing list and on the python issue tracker. It is still going through some changes as of april 2016, I think they decided to split it into two macros Py_SETREF and Py_XSETREF.

One motivation for it is to avoid a certain kind of bug, according to the comment in CPython. On the other hand it's not part of the "stable api", so there are some risks in using it.

ahaldane · 2017-11-07T20:54:44Z

Besides the small cleanup mentioned above, LGTM.

Implements a custom PyLong_FromLongDouble (with a different name) derived from PyLong_FromDouble. Fixes numpygh-9964

eric-wieser · 2017-11-08T04:06:25Z

Cleanup made, used the non-clashing name

ahaldane · 2017-11-08T14:34:10Z

LGTM, merging. Thanks Eric!

charris · 2017-11-10T17:16:28Z

Seems to fail the wheels build on the mac, https://travis-ci.org/MacPython/numpy-wheels.

EDIT: It's not obvious what the problem is, the Mac does support extended precision long doubles. Maybe a library function (clang vs gcc) or some such ...

charris · 2017-11-10T17:17:09Z

I think this indicates that we need to add Mac testing.

charris · 2017-11-10T17:36:40Z

numpy/core/src/private/npy_longdouble.c

+{
+    PyObject *v;
+    PyObject *l_chunk_size;
+    // number of bits to extract at a time. CPython uses 30, but that's because


Hmm, should be C style comment.

Careless, good spot. This is the reason for the Mac failure?

No. At the moment I'm suspecting the power function.

The problem is this line: huge_ld = 1234 * np.longdouble(2) ** exp. We need someone running on a Mac to check this out. Maybe a fallback double function is being used somewhere.

Here it is, powl is blacklisted on the Mac

/* powl gives zero division warning on OS X, see gh-8307 */ #if defined(HAVE_POWL) && defined(NPY_OS_DARWIN) #undef HAVE_POWL #endif

Although I don't see why that should be a problem, the double value of the fallback function should be OK with result cast to long double, so the multiplication should work. Maybe inlining is failing somewhere ...

Fired up the old Mac. The Mac double pow function errors with exp == 1024, Linux works with that. I'm simply going to use exp = 1023 as a fix in this case.

exp=1023 doesn't work, because then the result fits in double - the goal here is to produce a longdouble that exceeds the limits for a double.

I suppose using exp=1023 and multiplying by 2 would work

Yes, that is what I did. See #10000 . Oh, looky there, issue 10000, it's like watching the odometer turn over ...

Guess we will eventually find out if this all works for IBM double double.

ahaldane · 2018-01-20T05:51:46Z

numpy/core/tests/test_scalarmath.py

@@ -419,6 +420,21 @@ def test_clongdouble___int__(self):
            assert_raises(OverflowError, x.__int__)
            assert_equal(len(sup.log), 1)

+    @dec.skipif(np.finfo(np.double) == np.finfo(np.longdouble))


This test fails on ppc64.

I think I'm going to add @dec.skipif(platform.machine().startswith("ppc64")) here.

eric-wieser added 00 - Bug 51 - In progress component: numpy._core labels Nov 6, 2017

eric-wieser force-pushed the longdouble_int branch 2 times, most recently from 1821d0c to 922fcb6 Compare November 6, 2017 06:16

eric-wieser mentioned this pull request Nov 6, 2017

COMPAT: PyPy calls clongdouble_int which raises a warning #9181

Merged

eric-wieser force-pushed the longdouble_int branch 7 times, most recently from a1830b5 to f10d9d3 Compare November 6, 2017 09:23

ahaldane reviewed Nov 6, 2017

View reviewed changes

eric-wieser force-pushed the longdouble_int branch 2 times, most recently from 7d8004c to ec36b2b Compare November 7, 2017 03:24

eric-wieser commented Nov 7, 2017

View reviewed changes

eric-wieser force-pushed the longdouble_int branch from ec36b2b to 3a60d85 Compare November 7, 2017 03:51

BUG: Fix casting from longdouble to long

e0fb739

Implements a custom PyLong_FromLongDouble (with a different name) derived from PyLong_FromDouble. Fixes numpygh-9964

eric-wieser force-pushed the longdouble_int branch from 3a60d85 to e0fb739 Compare November 8, 2017 03:19

eric-wieser removed the 51 - In progress label Nov 8, 2017

ahaldane merged commit eba2671 into numpy:master Nov 8, 2017

charris reviewed Nov 10, 2017

View reviewed changes

ahaldane mentioned this pull request Nov 12, 2017

Floating-point pow gives different results on different platforms #10010

Open

ahaldane reviewed Jan 20, 2018

View reviewed changes

This was referenced Feb 23, 2018

BUG: Relocation R_X86_64_PC32 against `npy_longdouble_to_PyLong' can not be used when making a shared object; recompile with -fPIC #10648

Closed

BUG: Fix missing NPY_VISIBILITY_HIDDEN on npy_longdouble_to_PyLong #10650

Merged

charris mentioned this pull request Mar 11, 2018

BUG: Fix missing NPY_VISIBILITY_HIDDEN on npy_longdouble_to_PyLong #10727

Merged

Uh oh!

BUG: Fix casting from longdouble to long #9971

BUG: Fix casting from longdouble to long #9971

Uh oh!

Conversation

eric-wieser commented Nov 6, 2017

Uh oh!

charris commented Nov 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahaldane Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahaldane Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahaldane commented Nov 7, 2017

Uh oh!

eric-wieser commented Nov 8, 2017

Uh oh!

ahaldane commented Nov 8, 2017

Uh oh!

charris commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charris commented Nov 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahaldane Nov 6, 2017 •

edited

Loading

ahaldane Nov 6, 2017 •

edited

Loading

charris commented Nov 10, 2017 •

edited

Loading

eric-wieser Nov 10, 2017 •

edited

Loading