-
-
Notifications
You must be signed in to change notification settings - Fork 545
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
implement elliptic-curve point addition for general rings #39036
implement elliptic-curve point addition for general rings #39036
Conversation
3d33aa9
to
dca53c3
Compare
Documentation preview for this PR (built with commit f077c1a; changes) is ready! 🎉 |
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.
First review pass. I was only checking for style/formatting. I'll find some time to look at the math later, this looks like a cool contribution.
A few of the test failures seem relevant. Can you investigate and fix them?
Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>
Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>
Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>
Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>
…_to_rings SageMath version 10.5, Release Date: 2024-12-04
Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>
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.
Some additional comments, requests for more detailed docstrings, etc.
Unless I missed it, it seems all your tests for this are over Zmod(N)? Since this is supposed to work for general rings, can you include a test with a more complicated ring?
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.
Additional comments for code cleanup
Co-authored-by: Vincent Macri <vincent.macri@ucalgary.ca>
Alright, I believe all your comments have been dealt with now. Thanks for the thorough review! |
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.
Looks good to me. Thanks for the contribution!
… rings Here we revive the effort from sagemath#33228 to implement point-addition formulas on elliptic curves over (more) general rings. This resolves sagemath#33228. URL: sagemath#39036 Reported by: Lorenz Panny Reviewer(s): Lorenz Panny, Vincent Macri
… rings Here we revive the effort from sagemath#33228 to implement point-addition formulas on elliptic curves over (more) general rings. This resolves sagemath#33228. URL: sagemath#39036 Reported by: Lorenz Panny Reviewer(s): Lorenz Panny, Vincent Macri
This makes meson build fails. (I guess the fact that meson build fails all the time makes nobody want to look at it) diff --git a/src/sage/schemes/elliptic_curves/meson.build b/src/sage/schemes/elliptic_curves/meson.build
index 3448c5d1c0a..5f12079dc03 100644
--- a/src/sage/schemes/elliptic_curves/meson.build
+++ b/src/sage/schemes/elliptic_curves/meson.build
@@ -1,6 +1,7 @@
py.install_sources(
'BSD.py',
'Qcurves.py',
+ 'addition_formulas_ring.py',
'all.py',
'cardinality.py',
'cm.py', Not sure if someone already made a pull request though. |
Here we revive the effort from #33228 to implement point-addition formulas on elliptic curves over (more) general rings.
This resolves #33228.