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

fix random doctest failure in EllipticCurveHom_velusqrt #34467

Closed
yyyyx4 opened this issue Sep 1, 2022 · 7 comments
Closed

fix random doctest failure in EllipticCurveHom_velusqrt #34467

yyyyx4 opened this issue Sep 1, 2022 · 7 comments

Comments

@yyyyx4
Copy link
Member

yyyyx4 commented Sep 1, 2022

As reported in #34303 comment:25, EllipticCurveHom_velusqrt accidentally computes an irrational isogeny for curves defined over GF(3) with 7 rational points. The reason is that the computation internally requires a point lying outside the kernel, which only in this one specific case will be defined over a cubic instead of a quadratic extension. This in turn implies the linear factors in the numerator of the isogeny won't match up in conjugate pairs, causing the problem.

Quick fix: Simply restrict the implementation to degrees ≥ 9. Since the older EllipticCurveIsogeny implementation is much faster for degrees this small anyway, this restriction won't be a problem for any remotely realistic code.

CC: @antonio-rojas

Component: elliptic curves

Author: Lorenz Panny

Branch/Commit: 467eda5

Reviewer: Kwankyu Lee

Issue created by migration from https://trac.sagemath.org/ticket/34467

@yyyyx4 yyyyx4 added this to the sage-9.7 milestone Sep 1, 2022
@kwankyu
Copy link
Collaborator

kwankyu commented Sep 15, 2022

comment:2

How about adding a doctest for this problematic case with an explanation and a link to this ticket?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 15, 2022

Changed commit from 0872312 to 467eda5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 15, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c71dc3fincrease minimum degree for Îlu formulas to 9
467eda5add failing example

@yyyyx4
Copy link
Member Author

yyyyx4 commented Sep 15, 2022

comment:4

Done.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 16, 2022

comment:5

Thanks. LGTM.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 16, 2022

Reviewer: Kwankyu Lee

@vbraun
Copy link
Member

vbraun commented Sep 20, 2022

Changed branch from public/increase_velusqrt_minimum_degree_to_9 to 467eda5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants