Divide exactly, or to a chosen precision - #69
Merged
Merged
Conversation
Divide computed its fractional part through a double, so it capped at roughly sixteen significant digits regardless of what the operands carried. On a type whose whole purpose is arbitrary precision that was the wrong ceiling. A quotient with a terminating decimal expansion is now produced exactly, however many digits that takes. A reduced fraction terminates in base ten when its denominator is a product of twos and fives, which is cheap to test and usually cheap to reject: 1 / 8 is 0.125, and 1 / 2^64 keeps all sixty-four decimal places rather than the seventeen a double allows. A repeating quotient is taken to the precision of the wider operand, never fewer than MinimumDivisionPrecision significant digits, with the last digit rounded half away from zero. Whatever the scaled division discards is worth less than one unit of the dropped digits, so it can never carry the rounding decision across the halfway mark; at most it turns an exact tie into something above it, which rounds the same way. A new overload takes the precision explicitly. This changes results. Three tests pinned the old behaviour and are updated: two divisions that were correct only as far as a double reached, and one that compared Exp against 1/e when both happened to be equally imprecise. Exp still routes through a double, so that comparison is now made at the precision Exp actually delivers. It can also produce fewer significant digits than before in one case. The old implementation computed the integer part of a quotient exactly and appended a double-precision fraction, so a quotient with a long integer part came out with more digits than the new default: 924880000e19 divided by -4713100000e-19 gave fifty-four digits where it now gives fifty. That was not a contract so much as a side effect, and an erratic one - the same code gave seventeen digits for a quotient near one, and hundreds for a quotient with a large exponent. A uniform floor with an explicit override is the more predictable trade, and callers who want the extra digits can ask for them. Cost, from the benchmark suite: division is about twice as fast at eight and thirty significant digits, because the double round trip through formatting and parsing is gone, and about three and a half times slower at two hundred, because it now computes two hundred digits instead of discarding them. Verified by checking every division in the differential fuzz harness against the exact rational value rather than against the old implementation: 4,944 terminating quotients exact to the digit and 32,347 repeating ones within half a unit of their last significant digit, inside 871,454 checks with no failures. Also moves the benchmark project's coverage exemption out of the shared .NET workflow and into the project itself. That workflow is duplicated per repository rather than generated from one template - KtsuBuild carries a near-identical copy - so an exclusion added there would have to be repeated everywhere and re-applied on every resync. SonarQubeExclude in the benchmark project travels with the project and needs no workflow to know benchmarks exist, so the shared workflow goes back to its canonical form. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018eTdSeGPQHGUKf9V3c2yXw
|
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.



Follow-up to #68, covering the two things left open there.
1. Division no longer caps at what a
doublecan holdDividecomputed its fractional part through adouble, so it stopped at roughly sixteen significant digits regardless of what the operands carried. On a type whose purpose is arbitrary precision, that was the wrong ceiling.A terminating quotient is now exact, however many digits that takes. A reduced fraction terminates in base ten when its denominator is a product of twos and fives — cheap to test, and usually cheap to reject:
That second one is the exact value of
2⁻⁶⁴, all sixty-four decimal places of it, where before it stopped after seventeen.A repeating quotient is taken to the precision of the wider operand, never fewer than
MinimumDivisionPrecision(50) significant digits, with the last digit rounded half away from zero:A new overload takes the precision explicitly:
On the rounding: whatever the scaled division discards is worth less than one unit of the dropped digits, so it can never carry the decision across the halfway mark — at most it turns an exact tie into something above it, which rounds the same way. That is why the remainder can be dropped rather than tracked.
This changes results
Three existing tests pinned the old behaviour and are updated: two divisions that were correct only as far as a
doublereached, andTestExpWithNegativePower, which comparedExp(-1)against1 / eback when both were equally imprecise.Expstill routes through adouble, so that comparison is now made at the precisionExpactually delivers — which is itself worth knowing.It can also produce fewer digits than before, in one case
Worth stating plainly rather than burying. The old implementation computed the integer part of a quotient exactly and appended a double-precision fraction, so a quotient with a long integer part came out with more significant digits than the new default:
That was a side effect rather than a contract, and an erratic one — the same code gave seventeen digits for a quotient near one and hundreds for a quotient with a large exponent. A uniform floor with an explicit override is the more predictable trade, and callers who want the extra digits can now ask for them. Across 1 000 random pairs measured against the exact rational value, the new result is closer 851 times and the old one 103.
Cost
From the committed benchmark suite:
DivideFaster at small sizes because the
doubleround trip through formatting and parsing is gone. Slower at 200 digits because it now computes two hundred digits instead of discarding them — the previous number was cheap for the same reason it was wrong.2. The benchmark coverage exemption moved out of the shared workflow
#68 added
**/*.Benchmarks/**/*tosonar.coverage.exclusionsin.github/workflows/dotnet.yml, and I flagged that it would be lost if that file were resynced. Looking into pushing it upstream: there is no upstream to push it to. The workflow is duplicated per repository rather than generated from a template —ktsu-dev/KtsuBuildcarries a copy differing only in comment wording, and neitherktsu-dev/Sdknorktsu-dev/.githubholds a canonical version. An exclusion added there would have to be repeated in every repository that gains a benchmark project, and re-applied on every resync.So the exemption moved into the benchmark project instead:
It travels with the project, needs no workflow to know benchmarks exist, and
dotnet.ymlgoes back to its canonical form — byte-identical on that line to KtsuBuild's copy. The trade is that benchmark C# is no longer analysed for code smells; it is tooling that handles no untrusted input, and the alternative was an edit that had to be maintained in ~40 repositories.If you would rather keep analysis and only drop the coverage requirement,
SonarQubeTestProjectdoes that instead — say the word and I will switch it.Testing
dotnet test— 228 passing, 8 new covering exact termination across eleven denominators, the full2⁻⁶⁴expansion round-tripping through multiplication, requested precision from 1 to 200 digits, half-away-from-zero rounding in both signs, precision never falling below the operands', rejection of non-positive precision, and zero dividends.Still open
Exp, andPowwith a non-integer power, still route through adouble. They are genuine approximations rather than an implementation shortcut, so giving them arbitrary precision means choosing and implementing series expansions — a larger piece of work, and a separate one.Separately:
RoundandReduceSignificanceround by adding a run of repeated fives, which rounds123.45to124rather than123.TestReduceSignificancepins that behaviour deliberately, so I have left it alone, but the new division rounds correctly and the two are now inconsistent. Worth a decision.🤖 Generated with Claude Code
https://claude.ai/code/session_018eTdSeGPQHGUKf9V3c2yXw
Generated by Claude Code