classification
Title: time.sleep() should support objects with __float__
Type: Stage: patch review
Components: Interpreter Core Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: AVINASH MISHRA, jdemeyer, josh.r, ncoghlan, remi.lapeyre, ronaldoussoren, serhiy.storchaka, vstinner
Priority: normal Keywords: patch, patch

Created on 2019-01-10 15:58 by jdemeyer, last changed 2019-08-13 14:03 by vstinner.

Pull Requests
URL Status Linked Edit
PR 11507 closed vstinner, 2019-01-10 16:49
PR 11507 closed vstinner, 2019-01-10 16:49
PR 11636 open jdemeyer, 2019-01-21 13:47
PR 11636 open jdemeyer, 2019-01-21 13:47
PR 11636 open jdemeyer, 2019-01-21 13:47
Messages (29)
msg333391 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-10 15:58
This used to work correctly in Python 2:

class Half(object):
    def __float__(self):
        return 0.5
import time
time.sleep(Half())

With Python 3.6, one gets instead

Traceback (most recent call last):
  File "test.py", line 6, in <module>
    time.sleep(Half())
TypeError: an integer is required (got type Half)
msg333395 - (view) Author: AVINASH MISHRA (AVINASH MISHRA) Date: 2019-01-10 16:14
hey i am a total newbie to open source contribution.
can you help me understand this issue and can i help solve this issue?
msg333396 - (view) Author: Rémi Lapeyre (remi.lapeyre) * Date: 2019-01-10 16:15
time.sleep() is probably not the only function to have such a bug.

Maybe __int__() should default to:

    def __int__(self):
        return int(self.__float__())

when __float__ is defined and not __int__.

Nick Coghlan suggested something similar for __int__ and __index__.
msg333397 - (view) Author: Rémi Lapeyre (remi.lapeyre) * Date: 2019-01-10 16:18
See #33039 for the proposed change to __int__.
msg333401 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-10 16:45
The problem comes from the private C function _PyTime_FromObject() of Python/pytime.c. This function must use the proper conversion to minimize the precision loss. Lib/test/test_time.py contains a lot of tests on conversions from different types and ensure that values are rounded correctly. See also my PEP 564 "Add new time functions with nanosecond resolution".

The correct code works for float and int (and maybe decimal.Decimal, I don't recall!), but it seems like it doesn't support types with __float__(). You have to explicitly cast such objects using float(value).

PyNumber_Float() can be used to convert arbitrary object to a float, but I'm not sure in which order the conversion should be tried to avoid/reduce precision loss during the conversion.

Example:

>>> x=2**53+1; x - int(float(x))
1

If we convert 'x' (int) to float, we introduce an error of 1.
msg333404 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-10 16:54
PR 11507 is not directly related to this issue, see bpo-26669. But I wrote this PR when trying to fix this issue :-)
msg333416 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-10 20:07
> I'm not sure in which order the conversion should be tried to avoid/reduce precision loss during the conversion.

I would suggest the order

1. __index__ to ensure exact conversion of exact integers
2. __float__ to ensure correct conversion of floating-point numbers
3. __int__
msg333547 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2019-01-13 07:50
Deriving __int__ from __float__ wouldn't be the right answer, as that can easily lead to unwanted OverflowError exceptions and other issues.

However, Jeroen's suggested order of checking __index__ then __float__ then __int__ in _PyTime_FromObject makes sense to me, as that addresses Victor's desire to use the most precise conversion available.
msg333550 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-01-13 08:52
This can cause a loss of precision for Decimal.

If we want to support other numerical types with loss in double rounding, the most reliable way is to represent them as fractions (x.as_integer_ratio() or (x.numerator, x.denominator)) and use precise integer arithmetic.
msg333554 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-13 12:39
> the most reliable way is to represent them as fractions (x.as_integer_ratio() or (x.numerator, x.denominator))

I don't think that we can rely on non-dunder names like that. They are not reserved names, so classes can give them any semantics that they like. This is not just hypothetical: SageMath for example uses numerator() and denominator() methods, not properties.

If you really want to go through with this, probably a special method like __as_integer_ratio__ should be defined.

Anyway, I personally consider the double rounding for time.sleep() a non-issue. We are not trying to write a precise math library here, nobody will complain about sleeping a femtosecond too long.
msg333555 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-13 12:53
> The correct code works for float and int (and maybe decimal.Decimal, I don't recall!)

Not for Decimal! In fact sleep(Decimal("0.99")) is interpreted as sleep(0) because __int__ is used to convert.
msg333961 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-18 13:49
My proposal vastly improves the situation for Decimal. I will write a PR for this and I hope that it won't be rejected just because it's not perfect.
msg333963 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-18 13:57
> Not for Decimal! In fact sleep(Decimal("0.99")) is interpreted as sleep(0) because __int__ is used to convert.

Oh oh. I didn't know that. It should be fixed.
msg333968 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-18 14:15
I guess I should wait until PR 11507 is merged, to avoid merge conflicts.
msg333969 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-18 14:16
No please don't wait for my PR 11507. I'm not sure that it's correct, and this bug is more important than NaN/inf :-)
msg334120 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-21 10:33
To avoid code duplication, it's tempting to merge _PyTime_FromObject and _PyTime_ObjectToDenominator

These two functions almost do the same, but not quite.
msg334121 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2019-01-21 10:41
As a late response to msg333416 and msg333547, I don't agree with looking at __index__ in _PyTime_FromObject.  

The __index__ method is used when an object can be used as the index for a sequence, but should not silently convert to int or float.  See <https://www.python.org/dev/peps/pep-0357/>.
msg334122 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-21 10:48
The motivation for PEP 357 was certainly using an object as the index for a sequence, but that's not the only use case.

In fact PEP 357 states "For example, the slot can be used any time Python requires an integer internally"

So despite the name __index__, I think that this is now the de facto standard for "convert the object (which is some kind of integer) to a Python int without loss of precision".
msg334125 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2019-01-21 11:04
Using __index__ here doesn't feel right, although I can't explain why yet.
msg334128 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-21 11:19
If __index__ doesn't "feel" right, what do you propose then to fix this issue, keeping in mind the concerns of https://bugs.python.org/issue35707#msg333401
msg334129 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-21 11:22
In other words: if we can only use __float__ and __int__, how do we know which one to use?
msg334131 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-21 11:30
> If we want to support other numerical types with loss in double rounding

Looking at the existing code, I can already see several double-rounding "bugs" in the code, so I wouldn't be too much concerned here...
msg334136 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2019-01-21 13:27
> In other words: if we can only use __float__ and __int__, how do we know which one to use?

I guess __index__. I've read the definition of object.__index__ in the data model documentation, and using __index__ for this conversion is fine. I need to educate my sense for when it's right to use this method...

Sorry about the noise.
msg334480 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-01-28 16:53
You've got a reference leak in your __index__ based paths. PyNumber_Index is returning a new reference (either to the existing obj, or a new one, if the existing obj isn't already an int). You never release this reference. Simplest fix is to make intobj top level, initialized to NULL, and Py_XDECREF it along the convert_from_int code path (you can't DECREF it in the index specific path because it needs to survive past the goto, since it's replacing obj).

I'm also mildly concerned by how duplicative the code becomes post-patch. If it's not a major performance hit (don't think it is; not even sure the API is even used anymore), perhaps just implement _PyTime_ObjectToTime_t as a wrapper for _PyTime_ObjectToDenominator (with a denominator of 2, so rounding simplifies to just 0 == round down, 1 == round up)?

Example:

int
_PyTime_ObjectToTime_t(PyObject *obj, time_t *sec, _PyTime_round_t round)
{
    long numerator;
    if (_PyTime_ObjectToDenominator(obj, sec, &numerator, 2, round) == 0) {
       if (numerator) {
           if (*sec == _Py_IntegralTypeMax(time_t)) {
               error_time_t_overflow();
               return -1;
           }
           ++*sec;
       }
       return 0;
    }
    return -1;
}

Sorry for not commenting on GitHub, but my work computer has a broken Firefox that GitHub no longer supports properly.
msg334486 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-28 18:14
> I'm also mildly concerned by how duplicative the code becomes post-patch.

I know, that's why I added that comment on GitHub.

> perhaps just implement _PyTime_ObjectToTime_t as a wrapper for _PyTime_ObjectToDenominator

Sure, but will that increase the chances of PR 11636 being accepted? Unless a core developer who is willing to merge that PR asks me that, I'd rather not add extra complications to that PR. (to be clear: I mean no offense, it's just that getting a CPython PR accepted is hard)
msg334509 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-01-29 11:45
> You've got a reference leak in your __index__ based paths.

Thanks for pointing that out. I fixed that now.
msg339123 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-03-29 16:03
Is anybody willing to review PR 11636?
msg349004 - (view) Author: Jeroen Demeyer (jdemeyer) * (Python triager) Date: 2019-08-04 20:47
> If we want to support other numerical types with loss in double rounding, the most reliable way is to represent them as fractions (x.as_integer_ratio() or (x.numerator, x.denominator))

See https://discuss.python.org/t/pep-3141-ratio-instead-of-numerator-denominator/2037/24?u=jdemeyer for a proposal to define __ratio__ for this.
msg349560 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-08-13 14:03
See also bpo-20861.
History
Date User Action Args
2019-08-13 14:03:43vstinnersetkeywords: patch, patch

messages: + msg349560
2019-08-04 20:47:29jdemeyersetmessages: + msg349004
2019-03-29 16:03:46jdemeyersetmessages: + msg339123
2019-01-29 11:45:58jdemeyersetmessages: + msg334509
2019-01-28 18:14:58jdemeyersetmessages: + msg334486
2019-01-28 16:53:11josh.rsetkeywords: patch, patch
nosy: + josh.r
messages: + msg334480

2019-01-25 15:03:35jdemeyersetcomponents: + Interpreter Core
2019-01-21 13:47:50jdemeyersetpull_requests: + pull_request11409
2019-01-21 13:47:34jdemeyersetpull_requests: + pull_request11408
2019-01-21 13:47:19jdemeyersetpull_requests: + pull_request11407
2019-01-21 13:27:13ronaldoussorensetkeywords: patch, patch

messages: + msg334136
2019-01-21 11:30:32jdemeyersetmessages: + msg334131
2019-01-21 11:22:58jdemeyersetmessages: + msg334129
2019-01-21 11:19:22jdemeyersetmessages: + msg334128
2019-01-21 11:04:25ronaldoussorensetkeywords: patch, patch

messages: + msg334125
2019-01-21 10:48:34jdemeyersetmessages: + msg334122
2019-01-21 10:41:51ronaldoussorensetkeywords: patch, patch
nosy: + ronaldoussoren
messages: + msg334121

2019-01-21 10:33:01jdemeyersetmessages: + msg334120
2019-01-18 14:16:53vstinnersetkeywords: patch, patch

messages: + msg333969
2019-01-18 14:15:02jdemeyersetmessages: + msg333968
2019-01-18 13:57:24vstinnersetkeywords: patch, patch

messages: + msg333963
2019-01-18 13:49:03jdemeyersetmessages: + msg333961
2019-01-13 12:53:44jdemeyersetmessages: + msg333555
2019-01-13 12:39:24jdemeyersetmessages: + msg333554
2019-01-13 08:52:30serhiy.storchakasetkeywords: patch, patch
nosy: + serhiy.storchaka
messages: + msg333550

2019-01-13 07:50:30ncoghlansetkeywords: patch, patch

messages: + msg333547
2019-01-10 20:07:49jdemeyersetmessages: + msg333416
2019-01-10 16:54:43vstinnersetkeywords: patch, patch

messages: + msg333404
2019-01-10 16:49:38vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request11054
2019-01-10 16:49:31vstinnersetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11053
2019-01-10 16:45:52vstinnersetnosy: + vstinner
messages: + msg333401
2019-01-10 16:18:04remi.lapeyresetmessages: + msg333397
2019-01-10 16:17:23remi.lapeyresetnosy: + ncoghlan
2019-01-10 16:15:00remi.lapeyresetnosy: + remi.lapeyre
messages: + msg333396
2019-01-10 16:14:21AVINASH MISHRAsetnosy: + AVINASH MISHRA
messages: + msg333395
2019-01-10 15:58:39jdemeyercreate