feat: implements issue #50 #51, Implement variable and degree utilities: …#78
feat: implements issue #50 #51, Implement variable and degree utilities: …#78FawadHa1der wants to merge 4 commits intoVerified-zkEVM:masterfrom
Conversation
…ee utilities: vars and degrees
🤖 Gemini PR SummaryThis Pull Request implements variable and degree utility functions for the Features
Refactoring
Documentation
Analysis of Changes
✅ **Removed:** 3 `sorry`(s)
🎨 **Style Guide Adherence**Based on the provided style guide, the following lines violate the guidelines: CompPoly/Multivariate/CMvPolynomial.lean
CompPoly/Multivariate/CMvPolynomialLemmas.lean
📄 **Per-File Summaries**
Last updated: 2026-02-11 21:18 UTC. |
There was a problem hiding this comment.
Nice, thank you for this @FawadHa1der !! This looks very good to me. The typeclass change [CommSemiring R] to [Zero R] makes it more general, which is always welcome! :)
To avoid having too many "lemmas" files could we merge CMvPolynomialEvalLemmas.lean and CMvPolynomialVarsDegreesLemmas.lean into one file -- e.g. CMvPolynomialLemmas.lean -- for correctness lemmas and for the simp/grind set? It also might be worth marking mem_vars_iff_degreeOf_pos, degrees_zero, vars_zero, and degreeOf_zero with @[simp]. Mathlib also has a lemma degrees_one : degrees (1 : MvPolynomial σ R) = 0 that is marked with @[simp] - if it's easy to add, that would be nice (I can add it later, too).
And lastly re ticket guidelines - I would say on these tickets, treat them as guidelines but we are open to improvements to them, as long as they are computable and especially if they are more efficient/elegant! If there's a good reason for deviating from the ticket, am always open to it. In this case, I am happy with the alternative you suggest (I think it slightly matches mathlib better too):
def degrees {n : ℕ} {R : Type} [Zero R] (p : CMvPolynomial n R) : Multiset (Fin n) :=
Finset.sup (List.toFinset (List.map CMvMonomial.toFinsupp (Lawful.monomials p))) Finsupp.toMultiset
I'll also request the review of @Julek on this one since he has an interest in the multivariate polynomial implementation here
… simp/grind markers
|
Done, feedback incorporated. Please do check the application of the grind attribute. I understand the simp but not entirely clear on the application of grind. Thanks for answering my simplistic questions since I am relatively new to Lean but this should improve over time. |
dhsorens
left a comment
There was a problem hiding this comment.
no worries! This looks good to me, I'll give @Julek a few more days to review and okay before I merge. The simp/grind application looks good to me for now; building the right grind-set will probably be something that we do as we use these lemmas in the wild. There's a balance of adding enough to simplify proofs without adding so many that the tactic becomes slow.
|
/review External: Internal: Comments: |
🤖 AI Review (with external context)🤖 AI ReviewOverall Summary: 📄 **Review for `CompPoly.lean`**Implementation Analysis Hunk 1: Lines 7-13 import CompPoly.Multivariate.CMvMonomial
import CompPoly.Multivariate.CMvPolynomial
-import CompPoly.Multivariate.CMvPolynomialEvalLemmas
+import CompPoly.Multivariate.CMvPolynomialLemmas
import CompPoly.Multivariate.Lawful
import CompPoly.Multivariate.MvPolyEquiv
import CompPoly.Multivariate.Unlawful
Misformalization Checklist
Verdict & Feedback Verdict: Correct The change is a straightforward update to the module dependency graph. As the code compiles, the new import path is valid. This change indicates a refactor of the underlying file structure (likely renaming No changes are required. 📄 **Review for `CompPoly/Multivariate/CMvPolynomial.lean`**This review focuses on the changes in Verdict: The formalization is Correct. The changes are logically sound, consistent with the documentation, and adhere to standard patterns for polynomial libraries in Lean (e.g., behaving similarly to The following points detail the review findings: 1. Typeclass Relaxation for
|
the new degrees implementation could be slower since its using degreesOf implementation repeatedly, but trying to stay close to the ticket description.
the other implementation could be
def degrees {n : ℕ} {R : Type} [Zero R] (p : CMvPolynomial n R) : Multiset (Fin n) :=
Finset.sup (List.toFinset (List.map CMvMonomial.toFinsupp (Lawful.monomials p)))
Finsupp.toMultiset