Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#12768 closed enhancement (fixed)

Better plotting for isogeny graphs of elliptic curves, and handling of LMFDB labels

Reported by: roed Owned by: cremona
Priority: minor Milestone: sage-5.3
Component: elliptic curves Keywords:
Cc: cremona, kedlaya, vbraun Merged in: sage-5.3.beta0
Authors: David Roe, John Palmieri Reviewers: John Cremona, Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12769, #13109 Stopgaps:

Description (last modified by jhpalmieri)

There are only 17 different possible isogeny graphs for elliptic curves over Q. It would be nice if the isogeny graph was laid out in the same way each time, and if the labels corresponded to the Cremona labels of the curves in the isogeny class.

The second major topic handled by this ticket is to implement handling of LMFDB labels for elliptic curves as well as Cremona labels, and the conversions between these. the third is a new class for isogeny classes of elliptic curves over Q.

Apply: 12768.patch, 12768-review.patch, trac_12768-rebase-to-13109.patch

Attachments (4)

12768.patch (68.6 KB) - added by roed 8 years ago.
Ready for review
12768-review.patch (7.3 KB) - added by cremona 8 years ago.
Apply after previous
trac_12768-rebase-to-13109.patch (1.3 KB) - added by jhpalmieri 8 years ago.
trac_12768-rebase-to-13109.v2.patch (2.1 KB) - added by jhpalmieri 8 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 8 years ago by roed

  • Cc cremona kedlaya added

I still need to finish doctesting this, but I wanted to post it now since I'm not going to be able to work on it tomorrow. It adds the capability to make curves from LMFDB labels, to reorder isogeny classes, and makes the plot for the graph of an isogeny class deterministic. I have a corresponding patch to the LMFDB code that fixes all the problems I've observed with ordering and plotting.

Changed 8 years ago by roed

Ready for review

comment:2 Changed 8 years ago by roed

  • Status changed from new to needs_review

The tests in sage/schemes/elliptic_curves pass for me. Let's see what patchbot says.

comment:3 Changed 8 years ago by cremona

  • Authors set to David Roe
  • Description modified (diff)
  • Summary changed from Better plotting for isogeny graphs of elliptic curves to Better plotting for isogeny graphs of elliptic curves, and handling of LMFDB labels

comment:4 Changed 8 years ago by davidloeffler

Patchbot says "angry red blob", I'm afraid. Here's the problem:

patching file sage/databases/cremona.py
Hunk #5 FAILED at 792
1 out of 6 hunks FAILED -- saving rejects to file sage/databases/cremona.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 12768.patch

comment:5 Changed 8 years ago by davidloeffler

  • Status changed from needs_review to needs_work

comment:6 Changed 8 years ago by roed

  • Dependencies set to #12769
  • Status changed from needs_work to needs_review

Oops. Forgot a dependency.

comment:7 Changed 8 years ago by cremona

Patch applies fine after the one from #12769. Checking the code now.

comment:8 Changed 8 years ago by cremona

  • Description modified (diff)
  • Reviewers set to John Cremona

Review: This patch implements three different things: (1) A new class for isogeny classes of elliptic curves (currently only for curves over Q); (2) implementation of the new (March 2012) LMFDB labels for elliptic curves over Q, with utilities for comverting to and from Cremona labels; (3) better layouts for isogeny graphs of elliptic curves over Q.

I have been through the code in some detail and only found some minor things to fix, as in the reviewer's patch. One of these was a typo causing doctest failure. Nothing more than minor. If David R is happy with these, I'll give the ticket a positive review.

comment:9 Changed 8 years ago by roed

The changes all look fine to me, but the fourth hunk for ell_rational_field in the review patch fails to apply. I'll try to fix it later tonight, but if you get to it first go for it.

comment:10 Changed 8 years ago by roed

  • Status changed from needs_review to needs_work

comment:11 Changed 8 years ago by cremona

My patch was based on 5.0.beta13, if that helps. By the way, the reason I changed ell_generic to ell_rational_field was so that when testing a valid elliptic curve defined over a different field for membership of the isogenyclass one just gets False instead of an error as before; but I forgot to add some doctests to illustrate.

Changed 8 years ago by cremona

Apply after previous

comment:12 Changed 8 years ago by cremona

  • Status changed from needs_work to needs_review

I have updated the review patch: it used to be that when asking for order='database' for a curve not in the database, this was caught but the error message output itself raised a confusing error (on trying to output %self).  Instead the error message now uses %self.E.  I added a doctest which illustrates this.

I give David's original patch a positive review modulo my own reviewer's patch, so if David (or anyone) thinks my patch is OK, please give the whole ticket a positive review.  I tested this on 5.0.beta14.

comment:13 Changed 8 years ago by roed

  • Status changed from needs_review to positive_review

John's changes look fine to me.

comment:14 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

comment:15 Changed 8 years ago by jhpalmieri

  • Dependencies changed from #12769 to #12769, #13109
  • Status changed from positive_review to needs_work

Changed 8 years ago by jhpalmieri

comment:16 Changed 8 years ago by jhpalmieri

  • Cc vbraun added
  • Description modified (diff)
  • Status changed from needs_work to needs_review

Only the rebase patch needs to be reviewed.

comment:17 Changed 8 years ago by vbraun

  • Authors changed from David Roe to David Roe, John Palmieri
  • Reviewers changed from John Cremona to John Cremona, Volker Braun
  • Status changed from needs_review to positive_review

Looks good to me!

Changed 8 years ago by jhpalmieri

comment:18 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

Should I add a doctest for the deprecation? The 'v2' patch does that, so if you want to use that instead, change the ticket description accordingly.

comment:19 follow-up: Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.2 to sage-pending

comment:20 in reply to: ↑ 19 Changed 8 years ago by cremona

Replying to jdemeyer:

Why not 5.2?

comment:21 Changed 8 years ago by jdemeyer

Because of the long chain of dependencies which might not all be merged in sage-5.2. Maybe they will, but I can't guarantee.

comment:22 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.3

comment:23 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.3.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:24 follow-up: Changed 6 years ago by cremona

I think it is time that the deparecation message introduced here is removed.

comment:25 in reply to: ↑ 24 Changed 6 years ago by cremona

Replying to cremona:

I think it is time that the deprecation message introduced here is removed.

See #15694.

Note: See TracTickets for help on using tickets.