Repository navigation
timedelta.total_seconds needlessly inaccurate, especially for negative timedeltas #52890
Description
| BPO | 8644 |
|---|---|
| Nosy | @mdickinson, @abalkin, @pitrou |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2010-05-09.09:36:28.892>
created_at = <Date 2010-05-07.12:24:53.881>
labels = ['extension-modules', 'type-bug']
title = 'timedelta.total_seconds needlessly inaccurate, especially for negative timedeltas'
updated_at = <Date 2010-05-09.09:36:28.891>
user = 'https://github.com/mdickinson'bugs.python.org fields:
activity = <Date 2010-05-09.09:36:28.891>
actor = 'mark.dickinson'
assignee = 'none'
closed = True
closed_date = <Date 2010-05-09.09:36:28.892>
closer = 'mark.dickinson'
components = ['Extension Modules']
creation = <Date 2010-05-07.12:24:53.881>
creator = 'mark.dickinson'
dependencies = []
files = ['17255', '17267']
hgrepos = []
issue_num = 8644
keywords = ['patch']
message_count = 11.0
messages = ['105197', '105198', '105209', '105232', '105233', '105251', '105290', '105291', '105294', '105342', '105378']
nosy_count = 3.0
nosy_names = ['mark.dickinson', 'belopolsky', 'pitrou']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue8644'
versions = ['Python 2.7']
Activity
mdickinson commentedon May 7, 2010
I just noticed (while responding to bpo-8643) that timedelta.total_seconds() has some accuracy problems, especially for negative timedeltas:
This could be easily fixed by using integer arithmetic internally instead of float arithmetic:
This works especially nicely in combination with the new float repr and the improved accuracy of true division in 2.7 / 3.2, to give a total_seconds() whose repr is free of noise digits. (Well, for small timedeltas, anyway.)
Since total_seconds is new in 2.7 and 3.2, I think it would be good to fix this before the 2.7 release.
mdickinson commentedon May 7, 2010
P.S. This change would also make total_seconds() consistent with division by timedelta(seconds=1):
In fact, this might even be a good way to re-implement total_seconds internally.
abalkin commentedon May 7, 2010
I am attaching a patch for py3k branch. I am +1 for backporting to 2.7 and I avoided relying on py3k timedelta/timedelta in the patch. The tests and docs will need to be modified for the backport.
Technically speaking, this is a change in documented behavior for 2.7 because according to current docs, td.total_seconds() is equivalent to
td.microseconds / 1000000 + td.seconds + td.days * 24 * 3600. Therefore I would appreciate if reviewer made a decision on backport.mdickinson commentedon May 7, 2010
Thanks for the patch! Comments, in increasing order of fussiness:
(1) there should be a Py_DECREF(total_microseconds) in the case that the "PyLong_FromLong(1000000L)" call fails
(2) in the docstring, replace 'loose' by 'lose'
(3) also in the docstring, I suggest replacing "280 years" by "270 years": "td == timedelta(seconds = td.total_seconds())" starts failing at 2**33 seconds (around 272.2 Julian years) rather than 2**53 microseconds.
mdickinson commentedon May 7, 2010
Grr. s/docstring/docs/
abalkin commentedon May 8, 2010
Yes, regrtest -R is not a substitute for thinking.
Neither is spellcheck. :-)
Yes, and precision is not accuracy.
mdickinson commentedon May 8, 2010
Looks good to me.
I definitely think this should go into 2.7 as well; total_seconds has only seen the light of day in the beta releases so far, so it's unlikely we'd break anyone's code with this change. (And IMO this is a bugfix, not a feature.)
It might be polite to wait a day or two for the 2.7 backport, though: 2.7 beta 2 is supposed to be being released today.
mdickinson commentedon May 8, 2010
If you want a second (third?) opinion, we could ask Antoine.
mdickinson commentedon May 8, 2010
[Antoine declines to offer an opinion.]
Committed to py3k in r80979. I moved the Misc/NEWS entry from the 'Library' section to the 'Extension Modules' section.
Leaving open for the 2.7 backport.
abalkin commentedon May 8, 2010
On Sat, May 8, 2010 at 10:07 AM, Mark Dickinson <report@bugs.python.org> wrote:
..
Tracker web site seems to be down, so I am trying my luck attaching a
patch to an e-mail reply. It did work in the past.
The true division by 10**6 could probably be replaced with division by
1e6 in the tests and docs, but I am not sure if the two are always
equivalent.
mdickinson commentedon May 9, 2010
Perfect! Applied in r81020.
You're correct that n/10**6 and n/1e6 aren't the same thing, at least for n large enough:
In the second case, 2**53+1 first gets converted to a float, and then the division is performed, so there are two points at which a rounding error can be introduced. The first case only involves one rounding error.