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.

Sign up for GitHub

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

Jump to bottom

make EllipticCurve.lift_x() deterministic #35644

Merged
merged 4 commits into from
Jun 3, 2023
Merged

make EllipticCurve.lift_x() deterministic #35644

merged 4 commits into from
Jun 3, 2023

Conversation

yyyyx4
Copy link
Member

@yyyyx4 yyyyx4 commented May 15, 2023

Resolves #35281.

@yyyyx4 yyyyx4 requested a review from JohnCremona May 15, 2023 14:57
@yyyyx4 yyyyx4 mentioned this pull request May 15, 2023
Closed
1 task
JohnCremona
JohnCremona approved these changes May 15, 2023
View reviewed changes
Copy link
Member

@JohnCremona JohnCremona 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! See the comments on the liked issue for details.

@JohnCremona
Copy link
Member

Last doctest is fine, I also ran ptestlong and can confirm that that was the only outstanding one. I didn't know there were elliptic curves used in tests in rings/polynomial/multi_polynomial_ideal.py. This last one was just lazy, the author of the test could easily have given x and y coordinates!

@vbraun
Copy link
Member

vbraun commented May 27, 2023

Merge conflict

@yyyyx4 yyyyx4 force-pushed the public/make_lift_x_deterministic branch from bb60231 to 6da5d12 Compare May 29, 2023 08:14
@github-actions
Copy link

Documentation preview for this PR (built with commit 6da5d12) is ready! 🎉

JohnCremona
JohnCremona approved these changes May 29, 2023
View reviewed changes
Copy link
Member

@JohnCremona JohnCremona left a comment

Choose a reason for hiding this comment

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

I assume you just fixed the merge conflicts and this is the same otherwise

@yyyyx4
Copy link
Member Author

yyyyx4 commented May 29, 2023

Indeed, trivial rebase.

@vbraun vbraun merged commit d89ea60 into sagemath:develop Jun 3, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jun 3, 2023
@yyyyx4 yyyyx4 mentioned this pull request Sep 4, 2023
Open
@yyyyx4 yyyyx4 deleted the public/make_lift_x_deterministic branch October 8, 2023 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnCremona JohnCremona JohnCremona approved these changes

Assignees
No one assigned
Labels
c: elliptic curves p: minor / 4 t: enhancement
Projects
None yet
Milestone
sage-10.1
Development

Successfully merging this pull request may close these issues.

EllipticCurve.lift_x() is often deterministic, but not always
5 participants
@yyyyx4 @JohnCremona @vbraun @kwankyu @mkoeppe