-
Notifications
You must be signed in to change notification settings - Fork 384
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
[Merged by Bors] - feat(AlgebraicGeometry/EllipticCurve/DivisionPolynomial/Basic): define division polynomials #6703
Conversation
Multramate
commented
Aug 21, 2023
•
edited by alreadydone
Loading
edited by alreadydone
- depends on: [Merged by Bors] - feat(NumberTheory/EllipticDivisibilitySequence): expose the auxiliary sequence #10814
…acobian.Representative
…ve.Jacobian.Point
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.
Mostly LGTM, just a few comments
Mathlib/AlgebraicGeometry/EllipticCurve/DivisionPolynomial.lean
Outdated
Show resolved
Hide resolved
Mathlib/AlgebraicGeometry/EllipticCurve/DivisionPolynomial.lean
Outdated
Show resolved
Hide resolved
|
||
Furthermore, define the associated sequences $\phi_n, \omega_n \in R[X, Y]$ by | ||
* $\phi_n := X\psi_n^2 - \psi_{n + 1}\psi_{n - 1}$, and | ||
* $\omega_n := \tfrac{1}{2\psi_n} \cdot (\psi_{2n} - \psi_n^2(a_1\phi_n + a_3\psi_n^2)$. |
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.
Better write this as )
), and explain that how complEDS
).
Also explain that division by 2 is not possible in char 2, so either we need to divide in the universal ring over ℤ, or define
I think it makes sense to merge the definition part of my DivisionPolynomial.lean with this PR, and maybe call the file DivisionPolynomial/Defs.lean, but then you'll need to wait for my #13057. The proof of the smul formula can be put in a standalone file called DivisionPolynomial/Group.lean (or ZSMul.lean ??). The degree computations can be another file Degree.lean.
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'll fix the definition and add a short note explaining the issue with division by 2. I think it's a great idea to have multiple files since they're getting quite long. I'll propagate this to my downstream PRs.
I think we don't want to define
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 added a very short description of how to define Implementation Notes
we can add when we actually PR the definition of
Mathlib/AlgebraicGeometry/EllipticCurve/DivisionPolynomial.lean
Outdated
Show resolved
Hide resolved
Mathlib/AlgebraicGeometry/EllipticCurve/DivisionPolynomial.lean
Outdated
Show resolved
Hide resolved
PR summaryImport changesNo significant changes to the import graph
|
I only moved the files into a subdirectory in latest commit, while the changes reflecting the suggestions are in the previous commit. Note that there seems to be a weird bug with |
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 it would be nice to include the proofs that the uppercase and lowercase W.polynomial
. I think the useful statement is that there image under CoordinateRing.mk
are equal. (Since evalEval
at a point satisfying Equation
factors through CoordinateRing.mk
, the evalEval
of these congruent polynomials are also equal.)
Mathlib/AlgebraicGeometry/EllipticCurve/DivisionPolynomial/Basic.lean
Outdated
Show resolved
Hide resolved
Mathlib/AlgebraicGeometry/EllipticCurve/DivisionPolynomial/Basic.lean
Outdated
Show resolved
Hide resolved
Don't we need the |
Mathlib/AlgebraicGeometry/EllipticCurve/DivisionPolynomial/Basic.lean
Outdated
Show resolved
Hide resolved
Mathlib/AlgebraicGeometry/EllipticCurve/DivisionPolynomial/Basic.lean
Outdated
Show resolved
Hide resolved
Mathlib/AlgebraicGeometry/EllipticCurve/DivisionPolynomial/Basic.lean
Outdated
Show resolved
Hide resolved
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.
Thanks 🎉
maintainer merge
🚀 Pull request has been placed on the maintainer queue by alreadydone. |
This is great, thanks! bors merge |
…e division polynomials (#6703)
Pull request successfully merged into master. Build succeeded: |
* $\Phi_n := X\Psi_n^{[2]} - \tilde{\Psi}_{n + 1}\tilde{\Psi}_{n - 1}$ if $n$ is even, and | ||
* $\Phi_n := X\Psi_n^{[2]} - \tilde{\Psi}_{n + 1}\tilde{\Psi}_{n - 1}\Psi_2^{[2]}$ if $n$ is odd. |
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.
These two lines are broken in the documentation and I'm not sure why.
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.
Seems like it's an issue with _
being both part of markdown and LaTeX, see e.g. here.
…e division polynomials (#6703)