refactor(comp-poly): canonicalize polynomial APIs and tighten assumptions#154
refactor(comp-poly): canonicalize polynomial APIs and tighten assumptions#154
Conversation
Switch the computable polynomial carriers to semantic canonicality so they no longer depend on a particular `BEq`, and rebuild the bounded-degree transport directly so it stays computable. Add kernel-safe regression coverage for the updated univariate and bivariate APIs. Made-with: Cursor
Build Timing Report
Incremental Rebuild Signal
This compares a clean project build against an incremental rebuild in the same CI job; it is a lightweight variability signal, not a full cross-run benchmark. Slowest Current Clean-Build FilesShowing 20 slowest current targets, with comparison against the selected baseline when available.
|
🤖 Gemini PR SummaryMathematical Refactoring and Generalization
Computability and Kernel Stability
Documentation and Infrastructure
Status on Proof Completion:
Statistics
Lean Declarations ✏️ **Removed:** 12 declaration(s)
✏️ **Added:** 35 declaration(s)
✏️ **Affected:** 280 declaration(s) (line number changed)
🎨 **Style Guide Adherence**There are more than 20 violations of the style guide. They are grouped below by rule with representative examples:
📄 **Per-File Summaries**
Last updated: 2026-03-12 12:13 UTC. |
🤖 Initial AI review without external context🤖 AI ReviewOverall Summary: Executive SummaryTL;DR: Checklist Coverage: Key Lean 4 / Mathlib Issues:
Minor Polish & Nitpicks:
Overall Verdict: Changes Requested 📄 **Review for `CompPoly/Bivariate/Basic.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: (Self-Correction/Sanity Check during review): The PR successfully relaxes overly restrictive global typeclass constraints (like 📄 **Review for `CompPoly/Bivariate/ToPoly.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks:
📄 **Review for `CompPoly/Multilinear/Basic.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks:
📄 **Review for `CompPoly/Multilinear/Equiv.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: 📄 **Review for `CompPoly/Multivariate/CMvPolynomial.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: 📄 **Review for `CompPoly/Multivariate/Lawful.lean`**Verdict: Changes Requested Critical Misformalizations: Lean 4 / Mathlib Issues:
Nitpicks:
📄 **Review for `CompPoly/Multivariate/MvPolyEquiv/Instances.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: (The change to 📄 **Review for `CompPoly/Multivariate/Operations.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: 📄 **Review for `CompPoly/Univariate/Basic.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: (The refactoring of 📄 **Review for `CompPoly/Univariate/Linear.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: Reviewer Notes: 📄 **Review for `CompPoly/Univariate/Quotient.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks:
📄 **Review for `CompPoly/Univariate/QuotientEquiv.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: 📄 **Review for `CompPoly/Univariate/Raw/Core.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: Summary: The decoupling of the mathematical property 📄 **Review for `CompPoly/Univariate/Raw/Division.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: 📄 **Review for `CompPoly/Univariate/Raw/Ops.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: 📄 **Review for `CompPoly/Univariate/Raw/Proofs.lean`**Verdict: Needs Minor Revisions Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks:
📄 **Review for `CompPoly/Univariate/ToPoly/Core.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks:
📄 **Review for `CompPoly/Univariate/ToPoly/Degree.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks:
📄 **Review for `CompPoly/Univariate/ToPoly/Equiv.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: Feedback: Furthermore, weakening The strategy of shadowing the auto-inserted section variable Great work! 📄 **Review for `tests/CompPolyTests.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: 📄 **Review for `tests/CompPolyTests/Bivariate/Basic.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: 📄 **Review for `tests/CompPolyTests/Univariate/Linear.lean`**Verdict: Approved Critical Misformalizations: Lean 4 / Mathlib Issues: Nitpicks: |
Keep the instance-stability refactor decoupled from `BEq`, restore the missing bounded-degree subtype data instances, and expose the inherited `CBivariate` equality instances. Fold in the Lean style and test cleanups needed for the PR lint job to pass cleanly. Made-with: Cursor
|
Addressed the remaining Gemini follow-ups in
Local validation:
|
Push typeclass requirements down onto the raw and canonical polynomial APIs so computable declarations only require the structure they actually use. Update the univariate and bivariate bridge proofs to preserve the weaker signatures without regressing builds or tests. Made-with: Cursor
Tighten the most mechanical typeclass hypotheses on the raw univariate ops, bivariate core stubs, and multivariate core constructors so foundational APIs only require the structure they actually use. Keep the downstream bridge layer building by updating the one multivariate equivalence proof that unfolded the old autogenerated subtraction instance. Made-with: Cursor
Lower the univariate and bivariate toPoly bridges and the multilinear monomial-basis layer to semiring-level assumptions wherever the constructions only use additive and multiplicative structure. Keep the genuinely subtractive bridge facts explicitly ring-scoped so the API becomes tighter without changing behavior. Made-with: Cursor
Lower the remaining high-confidence bridge, quotient, and helper declarations to the weakest algebraic assumptions that match their implementations. Preserve existing inference behavior where mixed-arity multivariate fallbacks would otherwise compete with same-arity instances. Made-with: Cursor
Record the expectation that new Lean declarations should carry the weakest typeclass assumptions they actually need, avoid blanket section-level instance scopes, and point agents to concrete examples from the recent refactor. Made-with: Cursor
Remove the duplicated section variable in the multilinear monomial-basis helpers and generalize `CMlPolynomial.map` to the weaker semiring assumptions flagged in Gemini review. Made-with: Cursor
|
Addressed the remaining Gemini nits in
Local validation:
|
There was a problem hiding this comment.
this is kind of a huge PR that does a lot of things at once, and for that reason a bit hard to evaluate. In future, I would be grateful for more targeted PRs that are more feasible to evaluate. For large goals, incremental PRs should be the norm and we might even consider putting a standard expectation for max PR size. ~200–400 lines diff is ideal, once PRs exceed 500–800 lines, review quality drops significantly.
I see for e.g. that in CompPoly/Univariate/Linear.lean, bounded-degree objects were moved from being defined as a Submodule first to being defined as a Set predicate first, then the subtype ↥(degreeLT n) is given algebraic structure manually. The relevant definitions have been changed - what was the motivation for this? What does it give us? I assume that the definitions are still proved correct wrt Mathlib in ToPoly and so am not too fussed by them.
From what I can tell, this PR mainly does this from the PR summary
Transitions CPolynomial and CBivariate to a model tracking canonicity via an IsCanonical predicate (ensuring no trailing zeros). This enables core operations like coeff, degree, and size to function with minimal requirements such as Zero or BEq instead of full Semiring structures.
which I'm happy with especially since it removes the need for boolean equality across the board. I'm curious to know if IsCanonical will contribute to proof ergonomics in the upcoming projects and wary of implementing it if it is cumbersome to work with.
Lastly, NB we should probably consider merging (or no) #127 before a major refactor like this at the typeclass level.
|
Would it be possible to just make the |
|
@dhsorens I split this refactor into a smaller replacement series from fresh branches off current Replacement PRs
Notes
I’m closing this PR in favor of the smaller replacement stack above. |
Summary
native_decideregressions, and restore the affected computable bridge and bounded-degree APIs in a kernel-safe way.toPolybridges, and selected multivariate/multilinear helpers by removing blanket section scopes and pushing assumptions down onto individual declarations.Key Changes
CPolynomialandCBivariate, update raw/core canonicality lemmas, and make the canonical carriers more instance-stable.native_decidefrom the affected pathway and rebuild the relevantdegreeLT/toPolypieces so they remain computable and kernel-checked.CompPoly/Univariate/Raw/Core.lean,Raw/Ops.lean,Raw/Proofs.lean,Basic.lean, andLinear.leanto remove blanketvariabledeclarations and give each definition, instance, and theorem only the assumptions it actually uses.CompPoly/Univariate/Quotient.leanandQuotientEquiv.lean, including lowering the quotient carrier toZero, pushing add/mul/pow to the weaker algebraic boundary, and tightening the quotient-to-Polynomialbridge.CompPoly/Univariate/ToPoly/Core.lean,ToPoly/Degree.lean, andToPoly/Equiv.leanto semiring-level assumptions where possible, while keeping genuinely subtractive facts explicitly ring-scoped.CompPoly/Bivariate/Basic.leanandBivariate/ToPoly.lean, including semiring-level transport lemmas for coefficients, supports, degrees, evaluation, swap, and leading-coefficient results.CompPoly/Multivariate/{Operations,Lawful,CMvPolynomial,MvPolyEquiv/Instances}.leanandCompPoly/Multilinear/{Basic,Equiv}.lean, while preserving inference behavior for mixed-arity fallback instances.docs/wiki/typeclass-minimization.md, link it fromAGENTS.mdand the wiki index, and add/update regression coverage intests/CompPolyTests.lean,tests/CompPolyTests/Bivariate/Basic.lean, andtests/CompPolyTests/Univariate/Linear.lean.Testing
lake build CompPolyTestspython3 scripts/lint-style.py CompPoly/Univariate/Raw/Core.lean CompPoly/Univariate/Raw/Ops.lean CompPoly/Univariate/Raw/Proofs.lean CompPoly/Univariate/Raw/Division.lean CompPoly/Univariate/Basic.lean CompPoly/Univariate/Linear.lean CompPoly/Univariate/Quotient.lean CompPoly/Univariate/QuotientEquiv.lean CompPoly/Univariate/ToPoly/Core.lean CompPoly/Univariate/ToPoly/Degree.lean CompPoly/Univariate/ToPoly/Equiv.lean CompPoly/Bivariate/ToPoly.lean CompPoly/Multivariate/Lawful.lean CompPoly/Multilinear/Basic.leanpython3 ./scripts/check-docs-integrity.pyLean Action CI,Lint Style,Docs Integrity,PR Review, andPR SummaryReview Guidance
Focus on:
native_decidepath.docs/wiki/typeclass-minimization.md, which documents the minimal-typeclass discipline used throughout this refactor.