Skip to content
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

Conversation

yyyyx4
Copy link
Member

@yyyyx4 yyyyx4 commented Nov 25, 2024

Here we revive the effort from #33228 to implement point-addition formulas on elliptic curves over (more) general rings.

This resolves #33228.

@yyyyx4 yyyyx4 force-pushed the public/generalize_elliptic_curve_point_addition_to_rings branch from 3d33aa9 to dca53c3 Compare November 25, 2024 18:18
Copy link

github-actions bot commented Nov 25, 2024

Documentation preview for this PR (built with commit f077c1a; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Copy link
Contributor

@vincentmacri vincentmacri left a 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?

yyyyx4 and others added 8 commits December 4, 2024 14:51
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>
Copy link
Contributor

@vincentmacri vincentmacri left a 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?

Copy link
Contributor

@vincentmacri vincentmacri left a 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

yyyyx4 and others added 2 commits December 5, 2024 20:12
@yyyyx4
Copy link
Member Author

yyyyx4 commented Dec 8, 2024

Alright, I believe all your comments have been dealt with now. Thanks for the thorough review!

Copy link
Contributor

@vincentmacri vincentmacri left a 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!

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 11, 2024
… 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
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 12, 2024
… 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
@vbraun vbraun merged commit 2df4906 into sagemath:develop Dec 15, 2024
19 of 23 checks passed
@user202729
Copy link
Contributor

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.

@yyyyx4 yyyyx4 deleted the public/generalize_elliptic_curve_point_addition_to_rings branch December 18, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement elliptic-curve point addition for general rings
4 participants