Opened 6 years ago
Closed 6 years ago
#16806 closed enhancement (fixed)
Isogeny Bounds for Elliptic Curves over Number Fields
Reported by:  elarson3  Owned by:  

Priority:  minor  Milestone:  sage6.4 
Component:  elliptic curves  Keywords:  
Cc:  cremona, wuthrich  Merged in:  
Authors:  Eric Larson  Reviewers:  John Cremona, Chris Wuthrich 
Report Upstream:  N/A  Work issues:  
Branch:  8c93016 (Commits)  Commit:  8c9301671b354f7ba6c24d48f28d0e2b0698f39f 
Dependencies:  Stopgaps: 
Description
Change History (23)
comment:1 Changed 6 years ago by
 Branch set to u/elarson3/isogeny_bounds_for_elliptic_curves_over_number_fields
comment:2 Changed 6 years ago by
 Commit set to 00199fb220aa173d8585b9e90654dafd3247d82d
 Component changed from PLEASE CHANGE to elliptic curves
 Milestone changed from sage6.3 to sagefeature
 Priority changed from major to minor
 Status changed from new to needs_review
 Type changed from PLEASE CHANGE to enhancement
comment:3 Changed 6 years ago by
 Cc cremona added
comment:4 Changed 6 years ago by
 Commit changed from 00199fb220aa173d8585b9e90654dafd3247d82d to fdbb13305246209ffe9e6adf78968091c4b3e5ed
comment:5 Changed 6 years ago by
 Commit changed from fdbb13305246209ffe9e6adf78968091c4b3e5ed to ea410d47cdf63f26bf4b7dacf255b90b3f9d13d5
Branch pushed to git repo; I updated commit sha1. New commits:
ea410d4  Revert "Updated Sage version to 6.3"

comment:6 Changed 6 years ago by
 Commit changed from ea410d47cdf63f26bf4b7dacf255b90b3f9d13d5 to 4b1a2d4ece0914fff47dae5ee4210dcf5745c217
Branch pushed to git repo; I updated commit sha1. New commits:
4b1a2d4  Revert "Revert "Updated Sage version to 6.3""

comment:7 Changed 6 years ago by
 Commit changed from 4b1a2d4ece0914fff47dae5ee4210dcf5745c217 to 12c6f0162ff02ab4800f969f23f40942c6852c0d
Branch pushed to git repo; I updated commit sha1. New commits:
12c6f01  comitting because git wants me to?

comment:8 Changed 6 years ago by
 Commit changed from 12c6f0162ff02ab4800f969f23f40942c6852c0d to dc0185c5b11d7ce7347bd4a724efc1bf7bf6a4f1
Branch pushed to git repo; I updated commit sha1. New commits:
dc0185c  sorted out now?

comment:9 Changed 6 years ago by
 Commit changed from dc0185c5b11d7ce7347bd4a724efc1bf7bf6a4f1 to 87718324fe607636d420d0fa25874d7e1de67009
Branch pushed to git repo; I updated commit sha1. New commits:
8771832  forgot to import Set

comment:10 Changed 6 years ago by
OK  branch now works, I successfully merged it into my own working branch for #16743 (which does not yet use this new code).
comment:11 Changed 6 years ago by
 Branch changed from u/elarson3/isogeny_bounds_for_elliptic_curves_over_number_fields to u/cremona/ticket/16806
 Commit changed from 87718324fe607636d420d0fa25874d7e1de67009 to 828ed6fee261a575c27e996d84c11c2bc7dabac0
 Reviewers set to John Cremona
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 reupload, but the changes I made should all be visible already in the commit 828ed6f.
New commits:
cadd58f  Merge remotetracking branch 'trac/u/elarson3/isogeny_bounds_for_elliptic_curves_over_number_fields' into larson

828ed6f  #16806: reviewer's patch

comment:12 Changed 6 years ago by
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 rereview 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.
comment:13 Changed 6 years ago by
 Cc wuthrich added
comment:14 followup: ↓ 15 Changed 6 years ago by
A few comments:
 The docstring of the main function reads ""Returns a list of primes
p
including all primes for which the modp
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 aValueError
, 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.
comment:15 in reply to: ↑ 14 Changed 6 years ago by
Replying to wuthrich:
A few comments:
 The docstring of the main function reads ""Returns a list of primes
p
including all primes for which the modp
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 aValueError
, 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...
comment:16 Changed 6 years ago by
 Commit changed from 828ed6fee261a575c27e996d84c11c2bc7dabac0 to be81f80ca53d936ef7fe763753d3e6a34c0bc944
comment:17 Changed 6 years ago by
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!
comment:18 Changed 6 years ago by
 Branch changed from u/cremona/ticket/16806 to u/wuthrich/ticket/16806
 Commit changed from be81f80ca53d936ef7fe763753d3e6a34c0bc944 to 8c9301671b354f7ba6c24d48f28d0e2b0698f39f
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

comment:19 Changed 6 years ago by
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!
comment:20 Changed 6 years ago by
 Reviewers changed from John Cremona to John Cremona, Chris Wuthrich
 Status changed from needs_review to positive_review
comment:21 Changed 6 years ago by
Thanks!
comment:22 Changed 6 years ago by
 Milestone changed from sagefeature to sage6.4
comment:23 Changed 6 years ago by
 Branch changed from u/wuthrich/ticket/16806 to 8c9301671b354f7ba6c24d48f28d0e2b0698f39f
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Added code to bound isogenies to the GaloisRepresentation class