Skip to content

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

Open
canblmz1 wants to merge 1 commit into
python:mainfrom
canblmz1:pydatetime-timedelta-float-precision
Open

canblmz1 wants to merge 1 commit into
python:mainfrom
canblmz1:pydatetime-timedelta-float-precision

Conversation

@canblmz1

@canblmz1 canblmz1 commented Oct 28, 2026 •

Copy link
Copy Markdown

_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 <noreply@anthropic.com>
@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
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
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
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
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants