-
Notifications
You must be signed in to change notification settings - Fork 4
Adding algebraic properties of CPolynomial in Basic.lean #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding algebraic properties of CPolynomial in Basic.lean #64
Conversation
these typeclass instances are not satisfied by CPolynomial
these theorems characterize some of the important algebraic properties satisfied by CPolynomial
the mul funciton was changed so that the initial accumulator is now the empty array and not [0]. When the initial accumulator is [0] this causes odd behavior with multiplication, for example [0]*[]=[] but []*[0]=[0] when the initial accumulator is [0]. This change is neccesary for some of the algebraic properties to hold.
🤖 Gemini PR SummaryThis Pull Request focuses on formalizing the algebraic properties of the Features
Refactoring
Fixes
Documentation
Analysis of Changes
✅ **Removed:** 4 `sorry`(s)
✏️ **Affected:** 1 `sorry`(s) (line number changed)
🎨 **Style Guide Adherence**Based on the provided style guide, here are the specific lines that violate the guidelines:
📄 **Per-File Summaries**
Last updated: 2026-02-03 18:17 UTC. |
…rties_clean_start
added proofs for smul_addRaw_distrib and smul_distrib_trim
added proof of right associativity and cleaned up some formatting
Reordered lemmas and theorems to better reflect, content, dependencies, and increase readbility.
|
At this point the major theorems have been added, the file is sorry free and has been organized. I think that the issues related to typelcasses in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@desmondcoles1 this looks great to me - the organization is good, the lemmas proved will be really nice for both the Quotient and the Canonical typeclasses; as you pointed out Canonical is a natural next step. Note the one comment on the misformalized lemma.
Something to consider (not necessarily for this PR) - a lot of these lemmas are the core computational primitives for raw polynomials and ideally we can automate use of them in future proofs. You'll probably find that the best way to go about doing things is to add them to the simp or grindset so that typeclass instance proofs in Canonical are mostly done by grind. Ideally we build up a good set of lemmas that grind can use so that the more obvious aspects of proofs of computation over the polynomials are done with grind.
|
/review External: Internal: Comments: |
🤖 AI Review (with external context)🤖 AI ReviewOverall Summary: 📄 **Review for `CompPoly/Univariate/Basic.lean`**This review covers the changes in VerdictIncorrect. The code contains a critical misformalization in the definition of Critical Issues1. Off-by-One Error in
Other Observations (Non-Critical)1. Removal of Ring Instances
2.
3. Proofs and Lemmas
SummaryThe file is well-structured regarding the algebraic proofs, but the 📄 **Review for `CompPoly/Univariate/Quotient.lean`**Review of General Assessment These changes appear to be maintenance updates aligning the proof scripts with a refactor in the definition of Detailed Check
Misformalization Checklist
Verdict & Feedback Formalization Correct. |
CompPoly/Univariate/Basic.lean
Outdated
| match p.lastNonzero with | ||
| | none => 0 | ||
| | some i => i.val + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the AI is right - that this should actually be some i => i.val. e.g. C r has degree zero and (C r).lastNonzero returns 0. I recognize that the definition of degree is probably wrong. Do you agree @quangvdao ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this and I think this issue has a little more going than just indexing. I do agree that the indexing is wrong, but this is a simple fix.
More significantly though, as in the docstrings, we want to mirror mathlib's definitions. So I believe that we should use the following definitions for degree and natdDegree:
/-- Return the degree of a `CPolynomial`. -/
def degree (p : CPolynomial R) : WithBot ℕ :=
match p.lastNonzero with
| none => ⊥
| some i => i.val
/-- Natural number degree of a `CPolynomial`.
Returns the degree as a natural number. For the zero polynomial, returns `0`.
This matches Mathlib's `Polynomial.natDegree` API.
-/
def natDegree (p : CPolynomial R) : ℕ :=
match p.lastNonzero with
| none => 0
| some i => i.val
This change causes a few issues, and I want to have some time to investigate them, and of course I want input for both of you. This issue about the definition of degree is also not important for the rest of the content of this PR. My suggestion for proceeding: I am going to remove this definition of natDegree and keep degree the way it is in the current version of the main branch. I will open a separate PR to deal with this issue, so at least we can close this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that sounds good, agreed with you here
dhsorens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, I'm happy with this! merging
This PR will address the underlying problem of issues: #33 #34 #35 #36 and should provide helpful theorems and lemmas for resolving issues: #37 #38 #39 #40 #41 #42. The issue
CPolynomial Rdoes not in general form a semiring or ring because it lacks additive and multiplicative identity, but facts about arithmetic inCPolynomial Rare still needed to provide proofs that related types are a certain typeclass instance. The aim is to verifying which properties hold and state the correct analogues of things that don't hold. For example we should have:theorem mul_assoc [LawfulBEq R] (p q r : CPolynomial R) : p * q * r = p * (q * r)and while it is not true thatp+0=0+p=pwe do have that:theorem zero_add_trim [LawfulBEq R] (p : CPolynomial R) : 0 + p = p.trimThis PR would also change the definition of
mul. The multiplication function was changed so that the initial accumulator is now the empty array and not[0]. When the initial accumulator is[0]this causes odd behavior with multiplication, for example[0]*[]=[] but []*[0]=[0]when the initial accumulator is[0]. This change is necessary for some of the algebraic properties to hold.This PR would also include a definition for
natDegree: