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

Adding methods .x() and .y() for EllipticCurvePoint #37092

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

r98inver
Copy link
Contributor

@r98inver r98inver commented Jan 18, 2024

Added the more natural notation P.x() and P.y() to access respectively the x and y coordinates of an EllipticCurvePoint. The behaviour mimics P.xy() and aims to replace P.xy()[0] and P.xy()[1].
Multiple instances of P.xy()[0] and P.xy()[1] are also replaced in related modules.

#sd123

Done with @gioella

Copy link

Documentation preview for this PR (built with commit 8923a6a; changes) is ready! 🎉

...
ZeroDivisionError: rational division by zero
"""
if self[2] == 1:
Copy link
Member

@yyyyx4 yyyyx4 Jan 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call a.is_one() is quite a lot faster than the test a == 1: The reason is that the latter spends time on converting the Python integer 1 into the parent of a.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I also modified the method .xy() both to keep them equal and for the same speed reason

Copy link
Member

@yyyyx4 yyyyx4 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. Thank you!

(The tests seem to be failing for unrelated reasons.)

@r98inver
Copy link
Contributor Author

Thanks!

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 24, 2024
    
Added the more natural notation P.x() and P.y() to access respectively
the x and y coordinates of an EllipticCurvePoint. The behaviour mimics
P.xy() and aims to replace P.xy()[0] and P.xy()[1].
Multiple instances of P.xy()[0] and P.xy()[1] are also replaced in
related modules.

#sd123

Done with @gioella
    
URL: sagemath#37092
Reported by: Riccardo Invernizzi
Reviewer(s): Lorenz Panny, Riccardo Invernizzi
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 27, 2024
    
Added the more natural notation P.x() and P.y() to access respectively
the x and y coordinates of an EllipticCurvePoint. The behaviour mimics
P.xy() and aims to replace P.xy()[0] and P.xy()[1].
Multiple instances of P.xy()[0] and P.xy()[1] are also replaced in
related modules.

#sd123

Done with @gioella
    
URL: sagemath#37092
Reported by: Riccardo Invernizzi
Reviewer(s): Lorenz Panny, Riccardo Invernizzi
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 29, 2024
    
Added the more natural notation P.x() and P.y() to access respectively
the x and y coordinates of an EllipticCurvePoint. The behaviour mimics
P.xy() and aims to replace P.xy()[0] and P.xy()[1].
Multiple instances of P.xy()[0] and P.xy()[1] are also replaced in
related modules.

#sd123

Done with @gioella
    
URL: sagemath#37092
Reported by: Riccardo Invernizzi
Reviewer(s): Lorenz Panny, Riccardo Invernizzi
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 30, 2024
    
Added the more natural notation P.x() and P.y() to access respectively
the x and y coordinates of an EllipticCurvePoint. The behaviour mimics
P.xy() and aims to replace P.xy()[0] and P.xy()[1].
Multiple instances of P.xy()[0] and P.xy()[1] are also replaced in
related modules.

#sd123

Done with @gioella
    
URL: sagemath#37092
Reported by: Riccardo Invernizzi
Reviewer(s): Lorenz Panny, Riccardo Invernizzi
@vbraun vbraun merged commit 02179e6 into sagemath:develop Feb 2, 2024
15 of 18 checks passed
@r98inver r98inver deleted the ec_point_xy branch February 5, 2024 15:16
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
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.

4 participants