Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

py3: strange behavior of sleep() on Sage types #26311

Open
embray opened this issue Sep 19, 2018 · 13 comments
Open

py3: strange behavior of sleep() on Sage types #26311

embray opened this issue Sep 19, 2018 · 13 comments

Comments

@embray
Copy link
Contributor

embray commented Sep 19, 2018

For some reason, passing Python's time.sleep() a Sage RealLiteral less than 1 returns more-or-less immediately:

sage: x = 0.5
sage: type(x)
<class 'sage.rings.real_mpfr.RealLiteral'>
sage: x2 = float(0.5)
sage: type(x2)
<class 'float'>
sage: timeit('sleep(x)')
625 loops, best of 3: 1 µs per loop
sage: timeit('sleep(x2)')
5 loops, best of 3: 500 ms per loop
sage: # One more time for the folks in back...
sage: timeit('sleep(x)')
625 loops, best of 3: 1.01 µs per loop

It's not just in the context of timeit either. 0.5 is long enough that if you do sleep(x2) directly you can feel the delay, whereas with sleep(x) there is much less or even no perceptible delay.''

However,

sage: timeit('sleep(1.0)')
5 loops, best of 3: 1 s per loop

as expected.

The problem appears to stem from the (relatively) new PyTime API, and in particular this function:

static int
_PyTime_FromObject(_PyTime_t *t, PyObject *obj, _PyTime_round_t round,
                   long unit_to_ns)
{
    if (PyFloat_Check(obj)) {
        double d;
        d = PyFloat_AsDouble(obj);
        if (Py_IS_NAN(d)) {
            PyErr_SetString(PyExc_ValueError, "Invalid value NaN (not a number)");
            return -1;
        }
        return _PyTime_FromFloatObject(t, d, round, unit_to_ns);
    }
    else {
        long long sec;
        Py_BUILD_ASSERT(sizeof(long long) <= sizeof(_PyTime_t));

        sec = PyLong_AsLongLong(obj);
        if (sec == -1 && PyErr_Occurred()) {
            if (PyErr_ExceptionMatches(PyExc_OverflowError))
                _PyTime_overflow();
            return -1;
        }

        if (_PyTime_check_mul_overflow(sec, unit_to_ns)) {
            _PyTime_overflow();
            return -1;
        }
        *t = sec * unit_to_ns;
        return 0;
    }
}

So it does not properly handle custom types that implement __float__.

Reported upstream:

In the meantime I don't see that there's much to be done except to always wrap Sage types in float() before passing them to time functions (or, more extreme, provide our own wrappers for common time functions...)

Upstream: Reported upstream. Developers acknowledge bug.

Component: python3

Issue created by migration from https://trac.sagemath.org/ticket/26311

@embray embray added this to the sage-8.4 milestone Sep 19, 2018
@embray

This comment has been minimized.

@vinklein
Copy link
Mannequin

vinklein mannequin commented Sep 20, 2018

comment:2

Thanks for the investigation.

@embray embray modified the milestones: sage-8.4, sage-8.5 Oct 28, 2018
@embray
Copy link
Contributor Author

embray commented Dec 28, 2018

comment:4

Retargeting some of my tickets (somewhat optimistically for now).

@embray embray modified the milestones: sage-8.5, sage-8.7 Dec 28, 2018
@jdemeyer
Copy link

Upstream: Reported upstream. No feedback yet.

@jdemeyer

This comment has been minimized.

@embray
Copy link
Contributor Author

embray commented Jan 10, 2019

comment:6

It feels like there's been a bad habit in CPython lately of needlessly breaking support for custom numerical types.

@jdemeyer
Copy link

Changed upstream from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

@embray
Copy link
Contributor Author

embray commented Jan 22, 2019

comment:8

Yeesh, that thread on bpo is a mess... :(

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

comment:9

Replying to @embray:

Yeesh, that thread on bpo is a mess... :(

Wait until you see the code. DRY clearly does not apply to the CPython code base: I had to make the same fix 3 times.

@embray
Copy link
Contributor Author

embray commented Jan 23, 2019

comment:10

Replying to @jdemeyer:

Replying to @embray:

Yeesh, that thread on bpo is a mess... :(

Wait until you see the code. DRY clearly does not apply to the CPython code base: I had to make the same fix 3 times.

Have you seen Sage's code base? ;)

@embray
Copy link
Contributor Author

embray commented Mar 25, 2019

comment:11

Removing most of the rest of my open tickets out of the 8.7 milestone, which should be closed.

@embray embray removed this from the sage-8.7 milestone Mar 25, 2019
@embray embray added the pending label Mar 25, 2019
@sheerluck
Copy link
Contributor

comment:12

Replying to @jdemeyer:

I had to make the same fix 3 times.

For this ticket 1 time is enough, solved with two line patch:

--- a/Python/pytime.c
+++ b/Python/pytime.c
@@ -404,9 +404,11 @@
 }
 
 static int
-_PyTime_FromObject(_PyTime_t *t, PyObject *obj, _PyTime_round_t round,
+_PyTime_FromObject(_PyTime_t *t, PyObject *xobj, _PyTime_round_t round,
                    long unit_to_ns)
 {
+    PyObject *obj;
+    obj = PyNumber_Float(xobj);
     if (PyFloat_Check(obj)) {
         double d;
         d = PyFloat_AsDouble(obj);

This patch solves only one thing:

TypeError: 'sage.rings.real_mpfr.RealLiteral' object cannot be interpreted as an integer

and only for time.sleep()

I understand that custom builded python is not for everyone.

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

No branches or pull requests

3 participants