msg333391 - (view) |
Author: Jeroen Demeyer (jdemeyer) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2019-03-29 16:03 |
Is anybody willing to review PR 11636?
|
msg349004 - (view) |
Author: Jeroen Demeyer (jdemeyer) * |
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) * |
Date: 2019-08-13 14:03 |
See also bpo-20861.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:10 | admin | set | github: 79888 |
2020-05-23 00:14:27 | cheryl.sabella | set | keywords:
patch, patch nosy:
+ belopolsky, p-ganssle
versions:
+ Python 3.10, - Python 3.6, Python 3.7, Python 3.8 |
2019-08-13 14:03:43 | vstinner | set | keywords:
patch, patch
messages:
+ msg349560 |
2019-08-04 20:47:29 | jdemeyer | set | messages:
+ msg349004 |
2019-03-29 16:03:46 | jdemeyer | set | messages:
+ msg339123 |
2019-01-29 11:45:58 | jdemeyer | set | messages:
+ msg334509 |
2019-01-28 18:14:58 | jdemeyer | set | messages:
+ msg334486 |
2019-01-28 16:53:11 | josh.r | set | keywords:
patch, patch nosy:
+ josh.r messages:
+ msg334480
|
2019-01-25 15:03:35 | jdemeyer | set | components:
+ Interpreter Core |
2019-01-21 13:47:50 | jdemeyer | set | pull_requests:
+ pull_request11409 |
2019-01-21 13:47:34 | jdemeyer | set | pull_requests:
+ pull_request11408 |
2019-01-21 13:47:19 | jdemeyer | set | pull_requests:
+ pull_request11407 |
2019-01-21 13:27:13 | ronaldoussoren | set | keywords:
patch, patch
messages:
+ msg334136 |
2019-01-21 11:30:32 | jdemeyer | set | messages:
+ msg334131 |
2019-01-21 11:22:58 | jdemeyer | set | messages:
+ msg334129 |
2019-01-21 11:19:22 | jdemeyer | set | messages:
+ msg334128 |
2019-01-21 11:04:25 | ronaldoussoren | set | keywords:
patch, patch
messages:
+ msg334125 |
2019-01-21 10:48:34 | jdemeyer | set | messages:
+ msg334122 |
2019-01-21 10:41:51 | ronaldoussoren | set | keywords:
patch, patch nosy:
+ ronaldoussoren messages:
+ msg334121
|
2019-01-21 10:33:01 | jdemeyer | set | messages:
+ msg334120 |
2019-01-18 14:16:53 | vstinner | set | keywords:
patch, patch
messages:
+ msg333969 |
2019-01-18 14:15:02 | jdemeyer | set | messages:
+ msg333968 |
2019-01-18 13:57:24 | vstinner | set | keywords:
patch, patch
messages:
+ msg333963 |
2019-01-18 13:49:03 | jdemeyer | set | messages:
+ msg333961 |
2019-01-13 12:53:44 | jdemeyer | set | messages:
+ msg333555 |
2019-01-13 12:39:24 | jdemeyer | set | messages:
+ msg333554 |
2019-01-13 08:52:30 | serhiy.storchaka | set | keywords:
patch, patch nosy:
+ serhiy.storchaka messages:
+ msg333550
|
2019-01-13 07:50:30 | ncoghlan | set | keywords:
patch, patch
messages:
+ msg333547 |
2019-01-10 20:07:49 | jdemeyer | set | messages:
+ msg333416 |
2019-01-10 16:54:43 | vstinner | set | keywords:
patch, patch
messages:
+ msg333404 |
2019-01-10 16:49:38 | vstinner | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request11054 |
2019-01-10 16:49:31 | vstinner | set | keywords:
+ patch stage: (no value) pull_requests:
+ pull_request11053 |
2019-01-10 16:45:52 | vstinner | set | nosy:
+ vstinner messages:
+ msg333401
|
2019-01-10 16:18:04 | remi.lapeyre | set | messages:
+ msg333397 |
2019-01-10 16:17:23 | remi.lapeyre | set | nosy:
+ ncoghlan
|
2019-01-10 16:15:00 | remi.lapeyre | set | nosy:
+ remi.lapeyre messages:
+ msg333396
|
2019-01-10 16:14:21 | AVINASH MISHRA | set | nosy:
+ AVINASH MISHRA messages:
+ msg333395
|
2019-01-10 15:58:39 | jdemeyer | create | |