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

Bug in elliptic curve isogeny #19840

Closed
JohnCremona opened this issue Jan 6, 2016 · 20 comments
Closed

Bug in elliptic curve isogeny #19840

JohnCremona opened this issue Jan 6, 2016 · 20 comments

Comments

@JohnCremona
Copy link
Member

There is a bug in the code to compute 5-isogenies of elliptic curves of j-invariant 1728, when 5 is a square.

sage: K.<a> = NumberField(x^4 - 5*x^2 + 5)
sage: E = EllipticCurve([a^2 + a + 1, a^3 + a^2 + a + 1, a^2 + a,
17*a^3 + 34*a^2 - 16*a - 37, 54*a^3 + 105*a^2 - 66*a - 135])
sage: E.j_invariant()
1728
sage: K(5).is_square()
True
sage: E.isogenies_prime_degree(5)
ValueError: The polynomial does not define a finite subgroup of the elliptic curve.

or more directly

sage: from sage.schemes.elliptic_curves.isogeny_small_degree import isogenies_5_1728
sage: isogenies_5_1728(E)
ValueError: The polynomial does not define a finite subgroup of the elliptic curve.

I wrote this code about 5 years ago, and will fix it.

Depends on #19689

Component: elliptic curves

Keywords: isogenies

Author: John Cremona

Branch/Commit: 8ce0e65

Reviewer: Frédéric Chapoton

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

@JohnCremona
Copy link
Member Author

comment:1

I found the bug: on line 868, a*(beta**2-2)/6 should be (beta**2-2*a)/6.

The bug was not caught by the doctests since the only example where this code was tested had a=1.

@JohnCremona
Copy link
Member Author

comment:2

To review, run the code in the description before and after; note that a doctest is added with this example.


New commits:

a88c9a5#19840: isogeny bug fix and test

@JohnCremona
Copy link
Member Author

Branch: u/cremona/19840

@JohnCremona
Copy link
Member Author

Commit: a88c9a5

@JohnCremona
Copy link
Member Author

comment:3

There is a small merge conflict with #19689 so I will rebase this on that and make that ticket a dependency.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 7, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

ff0ba0c#19689: unit scaling for Weierstrass models of elliptic curves over number fields
80595c1#19840: isogeny bug fix and test

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 7, 2016

Changed commit from a88c9a5 to 80595c1

@JohnCremona
Copy link
Member Author

Dependencies: #19689

@fchapoton
Copy link
Contributor

comment:6

two details:

  • The following is not formatted correctly
See `trac``19840`:

but should be

See :trac:`19840`:
  • A typo in the first line: "with repect"

@JohnCremona
Copy link
Member Author

comment:7

Replying to @fchapoton:

two details:

  • The following is not formatted correctly
See `trac``19840`:

but should be

See :trac:`19840`:
  • A typo in the first line: "with repect"

Thanks, I am fixing those now and hope we can this into 7.0.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 12, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

e6ed40d#19840: correct typos after review

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 12, 2016

Changed commit from 80595c1 to e6ed40d

@JohnCremona
Copy link
Member Author

comment:9

Don't forget to fill in the "reviewer" box :)

@fchapoton
Copy link
Contributor

comment:10

sorry, still not formatted correctly, should be in fact

See :trac:`19840`::

with a double colon at the end.

Once done, you can set a positive review.

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@JohnCremona
Copy link
Member Author

comment:11

Sorry, will do. It is so hard to actually test correct formatting of docstrings since it takes a very long time to build doc and the output is thousamds of lines somewhere in which is the relevant line or two.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 13, 2016

Changed commit from e6ed40d to 8ce0e65

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 13, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

8ce0e65#19840 last typo

@JohnCremona
Copy link
Member Author

comment:14

7.0 please!

@vbraun
Copy link
Member

vbraun commented Jan 20, 2016

Changed branch from u/cremona/19840 to 8ce0e65

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

3 participants