Skip to content

Commit f941978

Browse files
Bound a gcd's intermediates separately from its inputs (#920) (#923)
PolynomialGcd.Gcd declined on inputs well inside every documented limit, because an intermediate of the subresultant remainder sequence went past MultivariatePolynomial.MaxTerms while neither input nor answer came close: left (b + c + 1) * (a + b) * (a + b + c + d) 19 terms right (a^2 + b*c + d) * (a + b + c + d) * (a + b + c + d) 29 terms gcd a + b + c + d 4 terms was declined A multivariate pseudo-remainder multiplies through by a leading coefficient that is itself a polynomial. The subresultant divisions bound how large the coefficients get -- which is what the class remark is about, and it holds -- but nothing bounds the monomial count, and three steps in a 25-term lead times a 159-term remainder passes 512. Raising MaxTerms is the one-line version and it is the wrong shape, which the measurement shows rather than the reasoning: at 2048 three documented refusals become answers, including TermCountsPastTheCeilingAreRefused, whose subject is a direct product of two 495-term inputs, and RefusedRatherThanAnsweredWrongly, whose input has 1001 terms and is meant to be stopped at the door. The bound on what may be *asked* was never too small. What was too small was the bound on what may be passed through on the way to an answer that is itself small. So MaxIntermediateTerms is a second constant, 4096, threaded as an optional argument through Multiply, Power and DivideExact and passed only by the gcd's remainder sequence. Everything reached any other way keeps MaxTerms by default, so the entry bound that protects the hot path is untouched and all three of those refusals still refuse. 3000 drawn triples 7 declined -> 0 declined the pair above declined -> a + b + c + d, checked as a greatest common divisor The sweep now asserts the count is zero rather than at most seven, so a refusal coming back fails there instead of passing quietly under a ceiling. Performance, which is the part #920 asked not be guessed at: TryCancel runs on every quotient the simplifier builds, so simpsweep is the hot path. Two runs at 4096 took 3m26.8s and 3m40.7s, one at 512 took 3m45.3s. The spread between repeats of the same setting is as large as the gap between the settings, so this supports no regression and does not support a speedup. The gcd test class is unchanged at 1 s. Verified: 6944 C# tests and 130 F# tests pass, 0 fail. propcheck 1340 checks 0 failures, simpsweep 10463/10463 agree 0 disagreements, rootcheck 596/596 clean, casbench 117/119 with 0 wrong, 0 error and 0 timeout. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 9d37492 commit f941978

3 files changed

Lines changed: 58 additions & 28 deletions

File tree

Sources/AngouriMath/Functions/Algebra/Polynomials/MultivariatePolynomial.cs

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,27 @@ internal sealed partial class MultivariatePolynomial
6363
/// </summary>
6464
internal const int MaxTerms = 512;
6565

66+
/// <summary>
67+
/// The ceiling for an intermediate of a calculation whose input and answer are both
68+
/// already inside <see cref="MaxTerms"/>.
69+
/// </summary>
70+
/// <remarks>
71+
/// A multivariate pseudo-remainder multiplies through by a leading coefficient that is
72+
/// itself a polynomial, so its intermediates grow in monomial count even when neither
73+
/// side is anywhere near the bound — the subresultant divisions bound how big the
74+
/// coefficients get, not how many terms there are. Holding those intermediates to the
75+
/// input bound refused a gcd of a 19-term and a 29-term pair.
76+
/// https://github.com/asc-community/AngouriMath/issues/920
77+
///
78+
/// It is deliberately a second constant rather than a larger <see cref="MaxTerms"/>.
79+
/// The input bound is what protects the hot path — <c>TryCancel</c> runs on every
80+
/// quotient the simplifier builds — and raising it turned three documented refusals
81+
/// into answers, including a direct product of two 495-term inputs. What was too small
82+
/// was never the bound on what may be asked, only the bound on what may be passed
83+
/// through on the way to an answer that is itself small.
84+
/// </remarks>
85+
internal const int MaxIntermediateTerms = 4096;
86+
6687
private const int BitsPerVariable = 8;
6788
private const ulong PowerMask = 0xFF;
6889

@@ -163,7 +184,7 @@ private static void Accumulate(Dictionary<ulong, ERational> into, ulong monomial
163184
into[monomial] = sum;
164185
}
165186

166-
internal MultivariatePolynomial? Multiply(MultivariatePolynomial other)
187+
internal MultivariatePolynomial? Multiply(MultivariatePolynomial other, int maxTerms = MaxTerms)
167188
{
168189
if (IsZero || other.IsZero)
169190
return Zero(VariableCount);
@@ -174,13 +195,13 @@ private static void Accumulate(Dictionary<ulong, ERational> into, ulong monomial
174195
if (!TryMultiplyMonomials(left.Key, right.Key, VariableCount, out var monomial))
175196
return null;
176197
Accumulate(result, monomial, left.Value.Multiply(right.Value).ToLowestTerms());
177-
if (result.Count > MaxTerms)
198+
if (result.Count > maxTerms)
178199
return null;
179200
}
180201
return new(VariableCount, result);
181202
}
182203

183-
internal MultivariatePolynomial? Power(int exponent)
204+
internal MultivariatePolynomial? Power(int exponent, int maxTerms = MaxTerms)
184205
{
185206
if (exponent < 0 || exponent > MaxDegree)
186207
return null;
@@ -190,14 +211,14 @@ private static void Accumulate(Dictionary<ulong, ERational> into, ulong monomial
190211
{
191212
if ((exponent & 1) == 1)
192213
{
193-
if (result.Multiply(square) is not { } multiplied)
214+
if (result.Multiply(square, maxTerms) is not { } multiplied)
194215
return null;
195216
result = multiplied;
196217
}
197218
exponent >>= 1;
198219
if (exponent == 0)
199220
break;
200-
if (square.Multiply(square) is not { } squared)
221+
if (square.Multiply(square, maxTerms) is not { } squared)
201222
return null;
202223
square = squared;
203224
}
@@ -292,7 +313,7 @@ internal MultivariatePolynomial LeadingCoefficientIn(int variable)
292313
/// That is the check the caller relies on: nothing is cancelled that has not been
293314
/// divided out and seen to leave nothing behind.
294315
/// </remarks>
295-
internal MultivariatePolynomial? DivideExact(MultivariatePolynomial divisor)
316+
internal MultivariatePolynomial? DivideExact(MultivariatePolynomial divisor, int maxTerms = MaxTerms)
296317
{
297318
if (divisor.IsZero)
298319
return null;
@@ -305,7 +326,7 @@ internal MultivariatePolynomial LeadingCoefficientIn(int variable)
305326
var divisorValue = divisor.terms[divisorLead];
306327
var quotient = new Dictionary<ulong, ERational>();
307328
var rest = this;
308-
for (var step = 0; step <= MaxTerms; step++)
329+
for (var step = 0; step <= maxTerms; step++)
309330
{
310331
if (rest.IsZero)
311332
return new(VariableCount, quotient);
@@ -317,7 +338,7 @@ internal MultivariatePolynomial LeadingCoefficientIn(int variable)
317338
if (divisor.MultiplyByTerm(monomial, value) is not { } product)
318339
return null;
319340
rest = rest.Subtract(product);
320-
if (rest.TermCount > MaxTerms)
341+
if (rest.TermCount > maxTerms)
321342
return null;
322343
}
323344
return null;

Sources/AngouriMath/Functions/Algebra/Polynomials/PolynomialGcd.cs

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -240,9 +240,9 @@ internal static bool TryCancel(Entity numerator, Entity denominator,
240240
// The division below is the whole point of the subresultant sequence: what
241241
// it leaves is a subresultant, so it comes out exact, and the coefficients
242242
// stay the size of the subresultants instead of compounding.
243-
if (scale.Power(delta) is not { } scalePower
244-
|| previousLead.Multiply(scalePower) is not { } factor
245-
|| remainder.DivideExact(factor) is not { } next)
243+
if (scale.Power(delta, MultivariatePolynomial.MaxIntermediateTerms) is not { } scalePower
244+
|| previousLead.Multiply(scalePower, MultivariatePolynomial.MaxIntermediateTerms) is not { } factor
245+
|| remainder.DivideExact(factor, MultivariatePolynomial.MaxIntermediateTerms) is not { } next)
246246
return null;
247247

248248
left = right;
@@ -252,9 +252,9 @@ internal static bool TryCancel(Entity numerator, Entity denominator,
252252
scale = previousLead;
253253
else if (delta > 1)
254254
{
255-
if (previousLead.Power(delta) is not { } raised
256-
|| scale.Power(delta - 1) is not { } divisor
257-
|| raised.DivideExact(divisor) is not { } updated)
255+
if (previousLead.Power(delta, MultivariatePolynomial.MaxIntermediateTerms) is not { } raised
256+
|| scale.Power(delta - 1, MultivariatePolynomial.MaxIntermediateTerms) is not { } divisor
257+
|| raised.DivideExact(divisor, MultivariatePolynomial.MaxIntermediateTerms) is not { } updated)
258258
return null;
259259
scale = updated;
260260
}
@@ -281,8 +281,8 @@ internal static bool TryCancel(Entity numerator, Entity denominator,
281281
break;
282282
MultithreadingFunctional.ExitIfCancelled();
283283
var shift = remainder.DegreeIn(main) - divisorDegree;
284-
if (divisorLead.Multiply(remainder) is not { } scaled
285-
|| remainder.LeadingCoefficientIn(main).Multiply(divisor) is not { } cancelling
284+
if (divisorLead.Multiply(remainder, MultivariatePolynomial.MaxIntermediateTerms) is not { } scaled
285+
|| remainder.LeadingCoefficientIn(main).Multiply(divisor, MultivariatePolynomial.MaxIntermediateTerms) is not { } cancelling
286286
|| cancelling.ShiftedBy(main, shift) is not { } shifted)
287287
return null;
288288
remainder = scaled.Subtract(shifted);
@@ -292,7 +292,7 @@ internal static bool TryCancel(Entity numerator, Entity denominator,
292292
return remainder;
293293
for (var i = 0; i < outstanding; i++)
294294
{
295-
if (divisorLead.Multiply(remainder) is not { } scaled)
295+
if (divisorLead.Multiply(remainder, MultivariatePolynomial.MaxIntermediateTerms) is not { } scaled)
296296
return null;
297297
remainder = scaled;
298298
}

Sources/Tests/UnitTests/Algebra/Polynomials/MultivariateGcdTest.cs

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -295,10 +295,13 @@ static int[] Draw(Random random)
295295
/// clock-seeded generator out.
296296
/// </summary>
297297
/// <remarks>
298-
/// Seven of these three thousand are declined rather than answered, and the count is
299-
/// asserted rather than ignored: <see cref="TheTermCeilingIsReachedByAnIntermediate"/>
300-
/// is what reaches the ceiling and why the inputs that do it are so small. The bound
301-
/// is an upper one, so finding fewer never fails.
298+
/// Seven of these three thousand used to be declined rather than answered, all of them
299+
/// for the reason in <see cref="AnIntermediatePastTheInputCeilingIsStillAnswered"/> —
300+
/// an intermediate past the input bound on the way to a small answer. Since
301+
/// <see cref="MultivariatePolynomial.MaxIntermediateTerms"/> that is none of them, and
302+
/// the count is asserted at zero rather than bounded, so a refusal reappearing fails
303+
/// here instead of passing quietly under a ceiling.
304+
/// https://github.com/asc-community/AngouriMath/issues/920
302305
/// </remarks>
303306
[Fact]
304307
public void GcdIsMultiplicativeOverManyDrawnTriples()
@@ -322,7 +325,7 @@ public void GcdIsMultiplicativeOverManyDrawnTriples()
322325
Assert.True(divisor.SameAs(expected), $"trial {trial}: the divisor is not the expected one");
323326
AssertIsGreatestCommonDivisor(first, second, divisor, SweepVariables.Length);
324327
}
325-
Assert.True(declined <= 7, $"{declined} of 3000 triples were declined");
328+
Assert.True(declined == 0, $"{declined} of 3000 triples were declined");
326329
}
327330

328331
#endregion
@@ -501,24 +504,30 @@ public void TermCountsPastTheCeilingAreRefused()
501504
/// wrong answer is that the step declines.
502505
/// </summary>
503506
/// <remarks>
504-
/// Pinned as a refusal, which is a legitimate answer. Should the ceiling be raised, or
505-
/// the sequence learn to keep its intermediates primitive, this becomes an answer and
506-
/// the test should be changed to assert that answer deliberately.
507+
/// This was pinned as a refusal, and is now the answer. The intermediate is held to
508+
/// <see cref="MultivariatePolynomial.MaxIntermediateTerms"/> rather than to the input
509+
/// bound, which is the distinction the refusal was missing: nothing about the question
510+
/// asked here is large, only something passed through on the way to it.
511+
/// https://github.com/asc-community/AngouriMath/issues/920
507512
/// </remarks>
508513
[Fact]
509-
public void TheTermCeilingIsReachedByAnIntermediate()
514+
public void AnIntermediatePastTheInputCeilingIsStillAnswered()
510515
{
511516
var variables = new[] { "a", "b", "c", "d" };
512517
var left = Polynomial("(b + c + 1) * (a + b) * (a + b + c + d)", variables);
513518
var right = Polynomial("(a ^ 2 + b * c + d) * (a + b + c + d) * (a + b + c + d)", variables);
514519
Assert.Equal(19, left.TermCount);
515520
Assert.Equal(29, right.TermCount);
516521

517-
// a + b + c + d divides both, and is not found.
522+
// a + b + c + d divides both, and is now found rather than declined.
518523
var common = Polynomial("a + b + c + d", variables);
519524
Assert.NotNull(left.DivideExact(common));
520525
Assert.NotNull(right.DivideExact(common));
521-
Assert.Null(Gcd(left, right, variables.Length));
526+
527+
var divisor = Gcd(left, right, variables.Length);
528+
Assert.NotNull(divisor);
529+
Assert.True(divisor.SameAs(common), "the divisor found is not a + b + c + d");
530+
AssertIsGreatestCommonDivisor(left, right, divisor, variables.Length);
522531
}
523532

524533
#endregion

0 commit comments

Comments
 (0)