Opened 5 years ago

Closed 5 years ago

#16806 closed enhancement (fixed)

Isogeny Bounds for Elliptic Curves over Number Fields

Reported by: elarson3 Owned by:
Priority: minor Milestone: sage-6.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 5 years ago by elarson3

  • Branch set to u/elarson3/isogeny_bounds_for_elliptic_curves_over_number_fields

comment:2 Changed 5 years ago by elarson3

  • Commit set to 00199fb220aa173d8585b9e90654dafd3247d82d
  • Component changed from PLEASE CHANGE to elliptic curves
  • Milestone changed from sage-6.3 to sage-feature
  • Priority changed from major to minor
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to enhancement

comment:3 Changed 5 years ago by cremona

  • Authors set to Eric Larson
  • Cc cremona added

comment:4 Changed 5 years ago by git

  • Commit changed from 00199fb220aa173d8585b9e90654dafd3247d82d to fdbb13305246209ffe9e6adf78968091c4b3e5ed

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

fdbb133Added code to bound isogenies to the GaloisRepresentation class

comment:5 Changed 5 years ago by git

  • Commit changed from fdbb13305246209ffe9e6adf78968091c4b3e5ed to ea410d47cdf63f26bf4b7dacf255b90b3f9d13d5

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

ea410d4Revert "Updated Sage version to 6.3"

comment:6 Changed 5 years ago by git

  • Commit changed from ea410d47cdf63f26bf4b7dacf255b90b3f9d13d5 to 4b1a2d4ece0914fff47dae5ee4210dcf5745c217

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

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

comment:7 Changed 5 years ago by git

  • Commit changed from 4b1a2d4ece0914fff47dae5ee4210dcf5745c217 to 12c6f0162ff02ab4800f969f23f40942c6852c0d

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

12c6f01comitting because git wants me to?

comment:8 Changed 5 years ago by git

  • Commit changed from 12c6f0162ff02ab4800f969f23f40942c6852c0d to dc0185c5b11d7ce7347bd4a724efc1bf7bf6a4f1

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

dc0185csorted out now?

comment:9 Changed 5 years ago by git

  • Commit changed from dc0185c5b11d7ce7347bd4a724efc1bf7bf6a4f1 to 87718324fe607636d420d0fa25874d7e1de67009

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

8771832forgot to import Set

comment:10 Changed 5 years ago by cremona

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 5 years ago by cremona

  • 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 re-upload, but the changes I made should all be visible already in the commit 828ed6f.


New commits:

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

comment:12 Changed 5 years ago by cremona

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.

Last edited 5 years ago by cremona (previous) (diff)

comment:13 Changed 5 years ago by wuthrich

  • Cc wuthrich added

comment:14 follow-up: Changed 5 years ago by wuthrich

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.

comment:15 in reply to: ↑ 14 Changed 5 years ago by cremona

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 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...

comment:16 Changed 5 years ago by git

  • Commit changed from 828ed6fee261a575c27e996d84c11c2bc7dabac0 to be81f80ca53d936ef7fe763753d3e6a34c0bc944

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

0f4e30bMerge branch 'develop' into larson
be81f80#16806: minor changes after review

comment:17 Changed 5 years ago by cremona

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 5 years ago by wuthrich

  • 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:

8c93016trac 16806: include in documentation

comment:19 Changed 5 years ago by cremona

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 5 years ago by wuthrich

  • Reviewers changed from John Cremona to John Cremona, Chris Wuthrich
  • Status changed from needs_review to positive_review

comment:21 Changed 5 years ago by cremona

Thanks!

comment:22 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-feature to sage-6.4

comment:23 Changed 5 years ago by vbraun

  • Branch changed from u/wuthrich/ticket/16806 to 8c9301671b354f7ba6c24d48f28d0e2b0698f39f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.