homepage

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)

hey i am a total newbie to open source contribution.
can you help me understand this issue and can i help solve this issue?

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

See #33039 for the proposed change to __int__.

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.

PR 11507 is not directly related to this issue, see bpo-26669. But I wrote this PR when trying to fix this issue :-)

> 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__

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.

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.

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

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

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.

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

I guess I should wait until PR 11507 is merged, to avoid merge conflicts.

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 :-)

To avoid code duplication, it's tempting to merge _PyTime_FromObject and _PyTime_ObjectToDenominator
These two functions almost do the same, but not quite.

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

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

Using __index__ here doesn't feel right, although I can't explain why yet.

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 

In other words: if we can only use __float__ and __int__, how do we know which one to use?

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

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

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.

> 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)

> You've got a reference leak in your __index__ based paths.
Thanks for pointing that out. I fixed that now.

Is anybody willing to review PR 11636?

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

See also bpo-20861.