Factorize uses the polynomial layer where no rule reaches (#1018) - #1094
Merged
Conversation
`Entity.Factorize` was composed entirely out of `RewriteRules`, so it factored what someone had written a rule for -- `x ^ 2 - 1` has one -- and handed everything else back whole. Square-free decomposition, Zassenhaus over Q, Kronecker's substitution and Hensel lifting were all in the tree and none of them on that path, so the operation whose entire job is factorisation was worse at it than the machinery that exists for it. `x ^ 3 - 1`, `x ^ 4 - 5x^2 + 4`, `x ^ 6 - 1`, `x ^ 7 - 1` and `x ^ 2 + 2x + 1` all factor now. Every case the rules already handled is byte-identical. **The layer speaks only where the rules said nothing.** The first version replaced their answers too, and that is wrong for a reason worth stating: the order two factors come out in is arbitrary, the rules' order is the one on record, and changing it changes answers that were never the complaint. A product coming out of the rules is kept as it is. **Simplify is unchanged, and that took a second seam.** `Simplificator` offers a factorisation as a *candidate* and the cost model decides. The metric prefers the expanded form -- `x ^ 6 - 1` rates 12 expanded against 58 factored -- so a factored candidate wins only where the two are closest, and those turn out to be exactly the places a factored answer is least wanted: `x ^ 3 / 3 + x ^ 2 / 2` became `(3 + 2 * x) * x ^ 2 / 6`, an antiderivative in a form nobody writes. That candidate site now asks `RuleBasedFactorizationAtLevel` for the rule-based half only. The issue says the cost-model objection does not apply to `Factorize`; that is incomplete, because `Factorize`'s output feeds the search, and 37 failing tests were the evidence. The transformation is total rather than declining, because `Then` has no notion of an optional part and a step returning null makes the whole chain decline. `PolynomialFactorization` is held in a nested class: static field initialisers run in declaration order and `Factorization` is eager, so a field would still be null when its pipeline is built -- which surfaces as `ArgumentNullException(nameof(next))` from a combinator and names nothing. `ExtensionsExampleTest.FactorizeString` recorded `x ^ 2 + 2 * x + 1` as `Factorize`'s answer to itself. It is `(x + 1) ^ 2` now, and that example is user-facing documentation.
Rafael-SOWNet
force-pushed
the
fix/factorize-uses-the-polynomial-layer
branch
from
August 27, 2026 15:18
1db7d26 to
75c78a8
Compare
Rafael-SOWNet
added a commit
that referenced
this pull request
Aug 27, 2026
#1094 asks the polynomial layer only where the rewrite rules said nothing, so that an answer the rules already gave is never replaced -- the order two factors come out in is arbitrary and theirs is the one on record. Declining a **product** outright was too broad for that goal. The rules take a numeric content out and hand back `2 * (x ^ 3 - 1)`. That is a product, so the layer declined it, and the remainder is exactly the shape #1018 is about. Every polynomial with a numeric content whose remainder needs the layer stopped one step short: `2 * x^3 - 2`, `3 * x^6 - 3`, `5 * x^7 - 5`. Each factor is asked separately now instead of the product being handed over whole. Every factor the rules found survives, and only the ones they could not split are split -- so `2 * x^4 - 10x^2 + 8`, which the rules factor in full, is unchanged, and so is `x * (1 + y)`. Found by asking of `Factorize` the property #1092 was about -- that the answer multiplies back to what it factored -- and noticing a row that *satisfied* it and was still less factored than it should be. The value check said nothing because `2 * (x ^ 3 - 1)` is perfectly equal to `2 * x ^ 3 - 2`; what was wrong was not the value but the stopping point, which is a thing a property test does not ask about unless it is told to.
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
1 participant
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.
Closes #1018.
Entity.Factorizewas composed entirely out ofRewriteRules, so it factored what someone hadwritten a rule for and handed everything else back whole — while square-free decomposition,
Zassenhaus over
Q, Kronecker's substitution and Hensel lifting all sat in the tree, unused bythe operation whose entire job is factorisation.
"x3 - 1".Factorize()x ^ 3 - 1(x - 1) * (x ^ 2 + x + 1)"x4 - 5x2 + 4".Factorize()x ^ 4 - 5 * x ^ 2 + 4(x + 1) * (x + 2) * (x - 2) * (x - 1)"x6 - 1".Factorize()x ^ 6 - 1"x7 - 1".Factorize()x ^ 7 - 1(x - 1) * (x ^ 6 + x ^ 5 + … + 1)"x2 + 2x + 1".Factorize()x ^ 2 + 2 * x + 1(x + 1) ^ 2"a2 - b2".Factorize()(a - b) * (a + b)"x4 - y4".Factorize()(x - y) * (x + y) * (x ^ 2 + y ^ 2)"x * y + x".Factorize()x * (1 + y)"sin(x) + 1".Factorize()sin(x) + 1Measured on a build of each side.
The layer speaks only where the rules said nothing
My first version replaced the rules' answers too, and that is wrong for a reason worth stating:
the order two factors come out in is arbitrary, the rules' order is the one on record, and
changing it changes answers that were never the complaint. A product coming out of the rules
is kept exactly as it is. That took the fallout from 9 changed expectations down to 1 — and the
one that remains is an improvement, not a reordering.
Simplifyis unchanged, and that took a second seamSimplificatoroffers a factorisation as a candidate and the cost model decides. The metricprefers the expanded form —
x ^ 6 - 1rates 12 expanded against 58 factored — so a factoredcandidate wins only where the two are closest, and those turn out to be exactly the places a
factored answer is least wanted:
That candidate site now asks
RuleBasedFactorizationAtLevelfor the rule-based half only.The issue says the cost-model objection does not apply to
Factorize. That is incomplete —Factorize's output feeds the search — and 37 failing tests were the evidence. Offering thelayer to that search is #746 tier 2's pluggable cost model, still open, and not this.
Two mechanical notes
The transformation is total rather than declining:
Thenhas no notion of an optional part,so a step returning
nullmakes the whole chain decline andTransformation.Factorization.Applyreturned nothing at all.
PolynomialFactorizationlives in a nested class. Static field initialisers run in declarationorder and
Factorizationis eager, so a plain field would still benullwhen its pipeline isbuilt — which surfaces as
ArgumentNullException(nameof(next))from a combinator and namesnothing about the real cause. This file has had that failure before.
One recorded verdict updated
ExtensionsExampleTest.FactorizeStringrecordedx ^ 2 + 2 * x + 1asFactorize's answer toitself. It is
(x + 1) ^ 2now — and that example is user-facing documentation.Depends on #1093, which fixes a wrong answer this change would otherwise have surfaced to a much
more used API.
Full suite: 8740 passed, 0 failed.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Bjumi5K7fg8yx6UK1mZTQd