Repository navigation
Give the three numeric types one remainder between them (#708) - #709
Merged
Rafael-SOWNet merged 1 commit intoAug 4, 2026
Merged
Rafael-SOWNet merged 1 commit into
Rafael-SOWNet merged 1 commit into
Conversation
They answered three different ways, and two of the answers were wrong: Integer threw ArithmeticException on a negative divisor, because EInteger.Mod refuses one. A public operator, on ordinary input. Real truncated, so it took the sign of the dividend where the other two took the sign of the divisor. Rational added the divisor whenever the truncated remainder came out negative, which is the right conversion only for a positive divisor. For a negative one it moved the answer away from zero, so (-7/2) % (-3) came back as -7/2 -- larger in magnitude than the divisor, and a remainder under no convention at all. The code carried a TODO saying as much. Integer and Rational are both Real, so which of the three applied depended on the static type at the call site rather than on the values. All three are now floored: the remainder takes the sign of the divisor, and a - b*floor(a/b) is what every one of them computes. Checked against SymPy 1.14 on all four sign pairs, integer and rational alike. It is also what this library already did by hand where it needed a remainder it could rely on -- ExpressionNumerical.Equality carries a private TrueRemainder that converts C#'s truncation into exactly this. For Integer and Rational only the broken cases move. Real is a behaviour change: -7 % 3 was -1 and is 2. 12 of the 27 new tests fail without it, two of them property tests -- that the remainder is smaller than the divisor, and that the difference is a whole multiple of it, which between them are the definition. Suite 4572 passed, 0 failed, with no internal call site disturbed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #709 +/- ##
==========================================
+ Coverage 80.99% 81.83% +0.83%
==========================================
Files 155 159 +4
Lines 13687 13807 +120
Branches 1957 2333 +376
==========================================
+ Hits 11086 11299 +213
+ Misses 1990 1854 -136
- Partials 611 654 +43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 #708.
The three numeric types answered
%three different ways, and two of the answers were wrong. BecauseIntegerandRationalare bothReal, which one applied depended on the static type at the call site rather than on the values.7 % 3-7 % 37 % -3-7 % -3IntegerbeforeRealbeforeRationalbefore (7/2 % 3etc.)IntegerusedEInteger.Mod, which refuses a negative divisor, soInteger.Create(7) % Integer.Create(-3)threwArithmeticException: Divisor is negative— a public operator throwing on ordinary input.Realtruncated, taking the sign of the dividend where the other two took the sign of the divisor.Rationaladded the divisor whenever the truncated remainder came out negative. That is the correct truncated-to-floored conversion only when the divisor is positive; for a negative one it moves the answer further from zero.(-7/2) % (-3)came back as-7/2— larger in magnitude than the divisor, so not a remainder under any convention. The code carried a// TODO: consider the case for the divisor to be negativesaying exactly this.Why floored
All three now compute
a - b*floor(a/b), so the remainder takes the sign of the divisor. This is what SymPy, Mathematica and Maxima answer, and the convention under which the residues modulo n are the numbers 0 to n-1.Checked against SymPy 1.14 rather than asserted, integer and rational alike:
It is also what the library already does by hand where it needs a remainder it can rely on —
ExpressionNumerical.Equality.cscarries a privateTrueRemainderthat converts C#'s truncation into precisely this.What actually changes for callers
For
IntegerandRational, only the broken cases move: the two that threw, and the one that was wrong.For
Realthis is a behaviour change —-7 % 3was-1and is now2. That is the point of the PR rather than a side effect, and it is why #708 is an issue rather than a quiet fix. Worth a maintainer's eye.Tests
27 new, of which 12 fail without the change. Two are property tests rather than tables, since between them they are the definition of a remainder:
Rationalcase was failing;a - (a % b)is a whole multiple ofb.Suite:
Failed: 0, Passed: 4572, Skipped: 14, Total: 4586, with no internal call site disturbed — the places that use%internally all divide by a positive constant.Independent of #703, which needs a remainder and computes the floored one itself rather than going through these operators. If both land, #703 could be simplified to use
%directly; I have left it alone so the two can be reviewed separately.