Isogeny Bounds for Elliptic Curves over Number Fields

elarson314 commented 9 years ago

CC: @JohnCremona @categorie

Component: elliptic curves

Author: Eric Larson

Branch/Commit: 8c93016

Reviewer: John Cremona, Chris Wuthrich

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

elarson314 commented 9 years ago

Branch: u/elarson3/isogeny_bounds_for_elliptic_curves_over_number_fields

elarson314 commented 9 years ago

Commit: 00199fb

JohnCremona commented 9 years ago

Author: Eric Larson

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

fdbb133 Added code to bound isogenies to the GaloisRepresentation class

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 00199fb to fdbb133

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from fdbb133 to ea410d4

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

ea410d4 Revert "Updated Sage version to 6.3"

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

4b1a2d4 Revert "Revert "Updated Sage version to 6.3""

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from ea410d4 to 4b1a2d4

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 4b1a2d4 to 12c6f01

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

12c6f01 comitting because git wants me to?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 12c6f01 to dc0185c

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

dc0185c sorted out now?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from dc0185c to 8771832

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

8771832 forgot to import Set

JohnCremona commented 9 years ago

comment:10

OK -- branch now works, I successfully merged it into my own working branch for #16743 (which does not yet use this new code).

JohnCremona commented 9 years ago

Reviewer: John Cremona

JohnCremona commented 9 years ago

Changed branch from u/elarson3/isogeny_bounds_for_elliptic_curves_over_number_fields to u/cremona/ticket/16806

JohnCremona commented 9 years ago

comment:11

I made some rather superficial changes to the new function and added a couple more examples. The changes simlify the code while retaining exactly the same logic. I had to rename the branch on trac (it is now u/cremona/ticket/16806) as that's the way it has to be (I cannot edit a u/elarson3 branch).

My edits were superficial, otherwise I give the original new code a positive review. That means that if you are happy with my additional changes we can jointly give it a positive review. Before that I'll merge in the current develop branch and re-upload, but the changes I made should all be visible already in the commit 828ed6f.

New commits:

`cadd58f`	`Merge remote-tracking branch 'trac/u/elarson3/isogeny_bounds_for_elliptic_curves_over_number_fields' into larson`
`828ed6f`	`#16806: reviewer's patch`

JohnCremona commented 9 years ago

Changed commit from 8771832 to 828ed6f

JohnCremona commented 9 years ago

comment:12

Forget the last part of my previous comment: I had already done the merge, as you can see from the list of new commits (first the merge, then my reviewer's patch). So this is ready for re-review by the original author!

I have now made #16743 dependent on this (isogeny classes of elliptic curves over number fields), which means that I am keener than ever to get this merged.

chriswuthrich commented 9 years ago

comment:14

A few comments:

The docstring of the main function reads ""Returns a list of primes p including all primes for which the mod-p representation might not be contained in a Borel."" Isn't it the complement, i.e., all those that "may be contained in a Borel" ?
I thought "try: ... except ValueError: return [0]" is not what we should do. If the long and convoluted line in try produces a ValueError, that could have many reasons, and we should not just ignore this and send back [0] but pass the error on.
It is a shame that this function will not be available for curves over Q. But I guess it should be a separate ticket to merge the two classes dealing with Galois Reps together at some point.

JohnCremona commented 9 years ago

comment:15

Replying to @categorie:

A few comments:

The docstring of the main function reads ""Returns a list of primes p including all primes for which the mod-p representation might not be contained in a Borel."" Isn't it the complement, i.e., all those that "may be contained in a Borel" ?

You are right.

I thought "try: ... except ValueError: return [0]" is not what we should do. If the long and convoluted line in try produces a ValueError, that could have many reasons, and we should not just ignore this and send back [0] but pass the error on.

I'm to blame for the long line -- the original version had this split over many lines. The set we return is the union of (1) primes of additive reduction (found as factors of gcd(c4,Delta) in effect; (2) primes ramified in the field; (3) the ones returned by _semistable_reducible_primes. The latter function uses [0] to denote CM. So I cannot see what circumstances will lead to a ValueError, and suggest we can simplify this.

It is a shame that this function will not be available for curves over Q. But I guess it should be a separate ticket to merge the two classes dealing with Galois Reps together at some point.

Agreed. I said so myself somewhere.

I will revise the branch to take into account your first two points...

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 828ed6f to be81f80

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

`0f4e30b`	`Merge branch 'develop' into larson`
`be81f80`	`#16806: minor changes after review`

JohnCremona commented 9 years ago

comment:17

I made cosmetic changes in line with Chris's comments: various tidying of docstrings (including fixing that double negative) and simplifying (making clearer I hope) the two long lines where various sets of primes are collected. It turned out that a try/except is needed since the function _semistable_reducible_primes() does raise a ValueError if given a CM curve.

Ready for a new review!

chriswuthrich commented 9 years ago

Changed commit from be81f80 to 8c93016

chriswuthrich commented 9 years ago

Changed branch from u/cremona/ticket/16806 to u/wuthrich/ticket/16806

chriswuthrich commented 9 years ago

comment:18

Am I right that the file does not appear in the reference manual ?

I added this and modified the starting docstring so that the reference manual builds.

Starting testing now. Except a positive review soon.

New commits:

8c93016 trac 16806: include in documentation

JohnCremona commented 9 years ago

comment:19

Thanks. I seem to have forgotten to check that building the docs worked, but that would have been null anyway until you added it to the manual. Good!

chriswuthrich commented 9 years ago

Changed reviewer from John Cremona to John Cremona, Chris Wuthrich

JohnCremona commented 9 years ago

comment:21

Thanks!

vbraun commented 9 years ago

Changed branch from u/wuthrich/ticket/16806 to 8c93016

sagemath / sage

Isogeny Bounds for Elliptic Curves over Number Fields #16806