Repository navigation
Bisect instead of halving when RootN's estimate overflows [patch] - #558
Open
matt-edmondson wants to merge 3 commits into
Open
matt-edmondson wants to merge 3 commits into
matt-edmondson wants to merge 3 commits into
Conversation
RootByDescent halved an estimate whose own power overflowed the type, on the assumption that the half stays at or above the root. Once the lift has doubled past the root that does not hold: RootN((short)15625, 6) halved 8 to 4, the Newton step from 4 did not descend, and the answer was 4, not 5. The descent now keeps the largest estimate known to be at or below the floor, and bisects between it and the overflowing estimate. Fixes #557 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW5ffCNUovhUpTzJZn7
Sonar's new-code coverage was 75%: no test reached the step that returns the floor once the bisection bounds are adjacent. Values one above an exact power do, so the sweep now checks k^d - 1, k^d and k^d + 1 for degrees 3 to 16, and the bisection's two-way update is a single assignment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW5ffCNUovhUpTzJZn7
SonarCloud S3776: RootByDescent's cognitive complexity was 16, one over the 15 allowed, after the bisection was added. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW5ffCNUovhUpTzJZn7
|
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.
Fixes #557
What was wrong
StorageMath.RootByDescentdepends on the estimate staying at or above the root. When the estimate'sdegree - 1power overflowed the type, it halved the estimate, assuming the half would still be at or above the root. Once the lift has doubled past the root, that assumption fails. ForRootN((short)15625, 6), the lift went 4 → 8,8⁵overflowedshort, and halving gave 4, which is below the root of 5. The Newton step from 4 did not descend, so the stale 4 was returned. The same happened for(short)16384at degree 7 and for 19⁷, 20⁷ and 21⁷ inint.Change
below, the largest estimate known to be at or below the floor of the root. It starts at one, and the lift raises it each time it doubles.belowand the estimate. If the two are adjacent,belowis the floor and is returned.RootBySettling(fractional types) andCbrtare unchanged.Tests
AnExactPowerInANarrowTypeRootsToItsRootcovers the issue's five cases, which now return 5, 4, 19, 20 and 21. It also covers two values where the bisection closes on the floor:(byte)244at degree 5, and 5¹³+1 at degree 13.TheNarrowIntegerTypesRootEveryExactPowerToItsFloorruns oversbyte,byte,short,ushort,intanduint. For degrees 3–16 it checks every kᵈ the type holds, plus kᵈ − 1 and kᵈ + 1, against aBigIntegerfloor root.StorageMath.cschange reverted, both tests fail.int/longvalues per degree, no input took the bisection's "midpoint is below the root" arm. It stays in because it is what keeps the bound correct.Semantics.Teston Linux / .NET 10: 2011 passed, 0 failed, 14 skipped (Windows-only path tests).🤖 Generated with Claude Code
https://claude.ai/code/session_017vfJW5ffCNUovhUpTzJZn7