Skip to content

gh-160806: Fix precision loss in pure-Python timedelta with large float arguments - #160807

Merged
StanFromIreland merged 2 commits into
python:mainfrom
canblmz1:pydatetime-timedelta-float-precision
Oct 29, 2026
Merged

StanFromIreland merged 2 commits into
python:mainfrom
canblmz1:pydatetime-timedelta-float-precision

Conversation

@canblmz1

@canblmz1 canblmz1 commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

_pydatetime.timedelta() added its float arguments together before splitting off the fractional part, so large values lost their sub-second part (timedelta(weeks=12345678.9) was off by 482 µs).

It now sums the microseconds the same way as the C implementation: the integer part of each argument exactly, and only the leftover fractional microseconds as a float, rounded half to even at the end. This also replaces the old step-by-step normalisation, which is much longer.

Since the arguments are now handled one by one in the same order as in C, inf/nan arguments also raise the same exception type as the C version (for example timedelta(days=inf, seconds=nan) raised OverflowError instead of ValueError).

Integer arguments are converted with int() first, so int subclasses with odd __mul__/__divmod__ (test_issue31752) still work.

🤖 Generated with Claude Code

…ge float arguments

_pydatetime.timedelta() added its float arguments together before
splitting off the fractional part, so large values lost their
sub-second part. Sum the microseconds like the C implementation does:
the integer part of each argument exactly, and only the leftover
fractional microseconds as a float, rounded half to even at the end.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@python-cla-bot

python-cla-bot Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@picnixz picnixz added the AI-needs-explanation AI-assisted work that needs further explanation and follow up from the contributor label Oct 28, 2026
@bedevere-app

bedevere-app Bot commented Oct 28, 2026

Copy link
Copy Markdown

Thank you for your interest in contributing to Python. This is a community-driven project, and we want it to be a welcoming place to learn and collaborate.

Contributors are responsible for reviewing and understanding what they submit, checking its claims, and being able to explain changes in their own words. This helps reviewers focus on the contribution and leaves more time to support other contributors.

This contribution appears to have been authored with AI assistance. Please explain your contribution in your own words and describe how you checked its correctness.

Please read our guidelines for using AI tools.

@canblmz1

Copy link
Copy Markdown
Contributor Author

Hello, I have reviewed the situation and verified the code in my local environment. The issue stems from a fundamental IEEE 754 floating-point precision loss:
Root Cause:
In the _pydatetime implementation, operations such as days += weeks * 7were performed directly using floating-point arithmetic. When the input weeks is a large value for instance, 12345678.9the result reaches approximately 86 million days. At this magnitude, the gap between consecutive representable values ​​(mantissa precision) in a 64-bit float expands to roughly 1.3 milliseconds. Consequently, data at the microsecond level is lost due to rounding errors before the fractional component can be accurately resolved, resulting in a deviation of 482 µs in the C implementation.The accum() logic on the C side functions as described in the documentation: it processes each argument individually, multiplying the integer portion using exact integer (Python int) arithmetic, while adding only the fractional component to a floating-point accumulator. Finally, it applies "round half to even" rounding at the microsecond stage.
The Fix:
I have aligned the Python implementation with the logic used in the C version,
I used math.modf to separate the integer and fractional parts of each argumentI scaled the integer parts using exact integer multiplication. summed the fractional parts and applied rounding at the end using _divide_and_round.To ensure compatibility with the custom/unusual integer subclasses used in test_issue31752, I passed the integer arguments through a standard nt() conversion.How I verified It I generated 100,000 random inputs and calculated absolute, precise reference values ​​using fractions.Fraction. Before the fix, the Python implementation showed discrepancies for approximately 19% of the inputs; after the fix, it matched the C implementation perfectly across all 100,000 test cases, with zero errors.I compiled the main branch locally in a Windows environment.The new tests I wrote fail on the main branch but pass successfully with the patch applied.I ran the existing test_datetime test suite in both pure Python and C modes; all tests passed completely
İ use the translate for that cause i have little bad english :(

@picnixz

picnixz commented Oct 28, 2026

Copy link
Copy Markdown
Member

I'm sorry but this still is a text entirely generated by an LLM and doesn't look like a comment from you that was simply translated. Note that we definitely do not want to simply talk to an LLM agent. The bug you reported is relevant, but the way the PR is described or solved doesn't ring like anything a human had actually written.

@canblmz1

Copy link
Copy Markdown
Contributor Author

the issue is that the time calculatedd in the pure pythın versioon differs from that of the c version due to a loss pf precision the orginal code multiplies the number of weeks by 7 first and with latge float values this operation can lead to additional loss of precision ın the new version we first separate the whole and fractional parts then convert them into microseconds this allow us to perform the calculation for the large integer portion using int

the reason we defer rouding until the very end is to ensure what the whole and fractional parts are evaluated together.For instance the sum of 1 microsecond and 0.5 microseconds is 1.5 which rounds up to 2 however if we were to round the 0.5 portion on its own flowing pyhons round to nearest even number rule it would become 0 consequently the final sum would be 1 instead of 2 and i didnt use ai in first one im sorry if its felt like that

@picnixz

picnixz commented Oct 28, 2026

Copy link
Copy Markdown
Member

will let you have the benefit of the doubt for this one, but in the future, if you do not necessarily know how to write the fix yourself without using an LLM agent, please do not open a PR. Because the PR is clearly written "Generated with Claude Code" which hints as a fully AI generated contribution.

@picnixz picnixz removed the AI-needs-explanation AI-assisted work that needs further explanation and follow up from the contributor label Oct 28, 2026
@canblmz1

Copy link
Copy Markdown
Contributor Author

I think there might have been a misunderstanding. I reviewed the code and explained it in my own words—could you point out where I went wrong? The rules actually permit the use of AI for writing code, provided that you explain it using your own sentences; did I misinterpret that? Thank you for your good intentions—you are far more knowledgeable than I am, and rest assured, I will follow your advice.

@picnixz

picnixz commented Oct 28, 2026

Copy link
Copy Markdown
Member

What we want to avoid in general:

  • People that only write a PR for having the title "Contributor" to an OSS repository. AI can be used but for getting knowledge around the code.
  • AIs are great at producing code but we want to keep contributors that would have anyway written that code by themselves. Understanding what the AI does is a first step, but being able to produce equivalent code would be preferrable.

Also, leaving the AI open a PR automatically, alongside with an unedited description, looks like "the agent did a PR but I understood the change, trust me". We are currently in a situation where we (at least I) need to doubt the good faith of a contributor because we don't know if they are just here for the "Contributor" badge or are really interested in helping.

For instance, in your first answer, the sentences have no ending periods, as if they were just C/C from a reply. The PR also makes a refactoring (This also replaces the old step-by-step normalisation, which is much longer.) which I believe is not necessary (it's hard to determine where the actual fix is with such change).

@canblmz1

Copy link
Copy Markdown
Contributor Author

Thank you very much for the explanation; you are absolutely right. Being a developer for a project in these times is very challenging you have to pay attention to everything. Many issues arise simply because people don't understand the code they are working with, and the approach you suggested encourages people to do a better job. I can close the PR if you’d like, and I’ll reopen it once I’ve gained enough knowledge to truly be trusted or, I’ll follow whatever you prefer. Thanks again, and please accept my apologies for my earlier behavior.

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid changing the entire logic and only change what needs to be changed? please don't use a loop and let the existing code remain as it contains relevant asseritions and comments. Only fix the parts that need to be fixed then.

Comment thread Lib/_pydatetime.py Outdated
@StanFromIreland

StanFromIreland commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

@picnixz I like this fix/refactor, we're now consistent with the C implementation's algorithm:

/* Fold in the value of the tag ("seconds", "weeks", etc) component of a
* timedelta constructor. sofar is the # of microseconds accounted for
* so far, and there are factor microseconds per current unit, the number
* of which is given by num. num * factor is added to sofar in a
* numerically careful way, and that's the result. Any fractional
* microseconds left over (this can happen if num is a float type) are
* added into *leftover.
* Note that there are many ways this can give an error (NULL) return.
*/
static PyObject *
accum(const char* tag, PyObject *sofar, PyObject *num, PyObject *factor,
double *leftover)
{
PyObject *prod;
PyObject *sum;
assert(num != NULL);
if (PyLong_Check(num)) {
prod = PyNumber_Multiply(num, factor);
if (prod == NULL)
return NULL;
sum = PyNumber_Add(sofar, prod);
Py_DECREF(prod);
return sum;
}
if (PyFloat_Check(num)) {
double dnum;
double fracpart;
double intpart;
PyObject *x;
PyObject *y;
/* The Plan: decompose num into an integer part and a
* fractional part, num = intpart + fracpart.
* Then num * factor ==
* intpart * factor + fracpart * factor
* and the LHS can be computed exactly in long arithmetic.
* The RHS is again broken into an int part and frac part.
* and the frac part is added into *leftover.
*/
dnum = PyFloat_AsDouble(num);
if (dnum == -1.0 && PyErr_Occurred())
return NULL;
fracpart = modf(dnum, &intpart);
x = PyLong_FromDouble(intpart);
if (x == NULL)
return NULL;
prod = PyNumber_Multiply(x, factor);
Py_DECREF(x);
if (prod == NULL)
return NULL;
sum = PyNumber_Add(sofar, prod);
Py_DECREF(prod);
if (sum == NULL)
return NULL;
if (fracpart == 0.0)
return sum;
/* So far we've lost no information. Dealing with the
* fractional part requires float arithmetic, and may
* lose a little info.
*/
assert(PyLong_CheckExact(factor));
dnum = PyLong_AsDouble(factor);
dnum *= fracpart;
fracpart = modf(dnum, &intpart);
x = PyLong_FromDouble(intpart);
if (x == NULL) {
Py_DECREF(sum);
return NULL;
}
y = PyNumber_Add(sum, x);
Py_DECREF(sum);
Py_DECREF(x);
*leftover += fracpart;
return y;
}
PyErr_Format(PyExc_TypeError,
"unsupported type for timedelta %s component: %s",
tag, Py_TYPE(num)->tp_name);
return NULL;
}
/*[clinic input]
@classmethod
datetime.timedelta.__new__ as delta_new
days: object(c_default="NULL") = 0
seconds: object(c_default="NULL") = 0
microseconds: object(c_default="NULL") = 0
milliseconds: object(c_default="NULL") = 0
minutes: object(c_default="NULL") = 0
hours: object(c_default="NULL") = 0
weeks: object(c_default="NULL") = 0
Difference between two datetime values.
All arguments are optional and default to 0.
Arguments may be integers or floats, and may be positive or negative.
[clinic start generated code]*/
static PyObject *
delta_new_impl(PyTypeObject *type, PyObject *days, PyObject *seconds,
PyObject *microseconds, PyObject *milliseconds,
PyObject *minutes, PyObject *hours, PyObject *weeks)
/*[clinic end generated code: output=61d7e02a92a97700 input=e8cd54819295d34b]*/
{
PyObject *self = NULL;
PyObject *current_mod;
datetime_state *st = GET_CURRENT_STATE(current_mod);
if (st == NULL) {
return NULL;
}
PyObject *x = NULL; /* running sum of microseconds */
PyObject *y = NULL; /* temp sum of microseconds */
double leftover_us = 0.0;
x = PyLong_FromLong(0);
if (x == NULL)
goto Done;
#define CLEANUP \
Py_DECREF(x); \
x = y; \
if (x == NULL) \
goto Done
if (microseconds) {
y = accum("microseconds", x, microseconds, _PyLong_GetOne(), &leftover_us);
CLEANUP;
}
if (milliseconds) {
y = accum("milliseconds", x, milliseconds, CONST_US_PER_MS(st), &leftover_us);
CLEANUP;
}
if (seconds) {
y = accum("seconds", x, seconds, CONST_US_PER_SECOND(st), &leftover_us);
CLEANUP;
}
if (minutes) {
y = accum("minutes", x, minutes, CONST_US_PER_MINUTE(st), &leftover_us);
CLEANUP;
}
if (hours) {
y = accum("hours", x, hours, CONST_US_PER_HOUR(st), &leftover_us);
CLEANUP;
}
if (days) {
y = accum("days", x, days, CONST_US_PER_DAY(st), &leftover_us);
CLEANUP;
}
if (weeks) {
y = accum("weeks", x, weeks, CONST_US_PER_WEEK(st), &leftover_us);
CLEANUP;
}
if (leftover_us) {
/* Round to nearest whole # of us, and add into x. */
double whole_us = round(leftover_us);
int x_is_odd;
PyObject *temp;
if (fabs(whole_us - leftover_us) == 0.5) {
/* We're exactly halfway between two integers. In order
* to do round-half-to-even, we must determine whether x
* is odd. Note that x is odd when it's last bit is 1. The
* code below uses bitwise and operation to check the last
* bit. */
temp = PyNumber_And(x, _PyLong_GetOne()); /* temp <- x & 1 */
if (temp == NULL) {
Py_DECREF(x);
goto Done;
}
x_is_odd = PyObject_IsTrue(temp);
Py_DECREF(temp);
if (x_is_odd == -1) {
Py_DECREF(x);
goto Done;
}
whole_us = 2.0 * round((leftover_us + x_is_odd) * 0.5) - x_is_odd;
}
temp = PyLong_FromLong((long)whole_us);
if (temp == NULL) {
Py_DECREF(x);
goto Done;
}
y = PyNumber_Add(x, temp);
Py_DECREF(temp);
CLEANUP;
}
self = microseconds_to_delta_ex(x, type);
Py_DECREF(x);
Done:
RELEASE_CURRENT_STATE(st, current_mod);
return self;
#undef CLEANUP
}

I think the new version is clearer (we don't have to "take a deep breath" half-way through ;-) and easier to maintain, so I'm happy to land this as-is (I applied a little consistency fix).

@StanFromIreland StanFromIreland added needs backport to 3.14 bugs and security fixes needs backport to 3.15 bugs and security fixes labels Oct 29, 2026
@StanFromIreland
StanFromIreland merged commit 2b6f543 into python:main Oct 29, 2026
61 checks passed
@miss-islington-app

Copy link
Copy Markdown

Thanks @canblmz1 for the PR, and @StanFromIreland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14, 3.15.
🐍🍒⛏🤖

@bedevere-app

bedevere-app Bot commented Oct 29, 2026

Copy link
Copy Markdown

GH-160889 is a backport of this pull request to the 3.15 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.15 bugs and security fixes label Oct 29, 2026
@bedevere-app

bedevere-app Bot commented Oct 29, 2026

Copy link
Copy Markdown

GH-160890 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Oct 29, 2026
@StanFromIreland

Copy link
Copy Markdown
Member

Merged, thanks!

StanFromIreland added a commit that referenced this pull request Oct 29, 2026
…large float arguments (GH-160807) (#160890)

(cherry picked from commit 2b6f543)

Co-authored-by: canblmz1 <[email protected]>
Co-authored-by: Stan Ulbrych <[email protected]>
StanFromIreland added a commit that referenced this pull request Oct 29, 2026
…large float arguments (GH-160807) (#160889)

(cherry picked from commit 2b6f543)

Co-authored-by: canblmz1 <[email protected]>
Co-authored-by: Stan Ulbrych <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants