Fix precisedelta rounding overflow across suppressed units - #417
Open
afonsojanu wants to merge 1 commit into
Open
afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
When a suppressed unit sits between the minimum_unit and the nearest unit that is still shown, an overflow caused by rounding the minimum_unit's value (e.g. 999.9999 milliseconds rounding up to 1000) had nowhere to carry into: the existing carry check only looked one level up and gave up as soon as that immediate neighbour was itself suppressed. For example, with seconds suppressed and milliseconds as the minimum unit, a value just under a full minute could render as "60000 milliseconds" instead of "1 minute", even though the exact same duration without suppression correctly rounds to "1 minute". The carry now compounds the conversion factor across any suppressed units until it reaches one that is actually displayed, matching how the unsuppressed case already behaves. Values that legitimately hold a large number in a unit because a higher unit was suppressed (e.g. 308 hours when days are suppressed) are unaffected, since they never reach the compounded threshold. Added regression tests for the single- and double-suppressed-unit carry, plus one confirming the legitimate large-value case is left untouched. Ran the existing suite (392 tests in test_time.py, 735 passing overall), ruff, ruff format and mypy --strict with no new issues, and differential-fuzzed precisedelta() against the previous implementation across 400k randomized inputs (uniform, boundary- adjacent, and multi-unit-suppressed combinations) with zero behavioral differences outside of this fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
precisedelta()has an overflow-correction step for when rounding pushes the minimum unit's displayed value up to (or past) the next unit's threshold, e.g. rounding 59.9 minutes to 60 minutes gets turned into "1 hour". That correction only ever looks one unit up, though, and gives up entirely if that neighbouring unit happens to be insuppress:Both calls represent the exact same duration, and 999937 microseconds rounds to 1000 milliseconds under
%0.0f, which is exactly what the existing carry logic is meant to catch. It works fine when seconds isn't suppressed, but as soon as it is, the carry has nowhere to go (the check that's supposed to promote it into minutes is gated onSECONDS not in suppress_set), so the rounding artifact just sits there as an invalid "60000 milliseconds" instead of folding into the next unit that's actually shown.It compounds across more than one suppressed unit too:
I changed the carry to keep compounding the conversion factor through any suppressed units above the minimum unit until it reaches one that's actually displayed, then applies the carry there. Units that legitimately hold a large count because a higher unit was suppressed (like 308 hours when
daysis suppressed) are unaffected, since a large value like that never reaches the compounded threshold — it's only the narrow rounding-boundary case that gets fixed.Added regression tests covering the single-suppressed-unit case, the two-level compounding case, and one confirming the legitimate "large value from a suppressed higher unit" case is untouched. The full existing suite still passes (392 tests in
test_time.py), along withruff,ruff format --check, andmypy --strict. I also differential-fuzzedprecisedelta()against the pre-fix implementation across ~400k randomized inputs (including values placed right at unit boundaries and varioussuppress/minimum_unitcombinations) and saw zero behavioral differences outside of the two scenarios this fixes.