Opened 9 years ago

Closed 9 years ago

#12766 closed enhancement (fixed)

Better plotting for elliptic curves

Reported by: roed Owned by: cremona
Priority: minor Milestone: sage-5.0
Component: elliptic curves Keywords:
Cc: wstein, cremona, kedlaya Merged in: sage-5.0.beta14
Authors: David Roe Reviewers: Kiran Kedlaya
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by roed)

The plot for EllipticCurve('448c6') looks like a vertical line. The reason is that plot contains

    d = 4*x**3 + (a1**2 + 4*a2)*x**2 + (2*a3*a1 + 4*a4)*x + (a3**2 + 4*a6)
    r = d.roots(multiplicities=False)
    if xmax is None:
        xmax = r[-1] + 2
    xmax = max(xmax, r[-1]+2)
    if xmin is None:
        xmin = r[0]  - 2
    xmin = min(xmin, r[0]-2)

When d has only one real root, this approach doesn't work that well. One suggestion would be to also require that the plot contains the flex points, which can be found from the 3-division polynomial.

Attachments (2)

12766.patch (8.1 KB) - added by roed 9 years ago.
12766_2.patch (3.2 KB) - added by roed 9 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 9 years ago by roed

  • Description modified (diff)

Changed 9 years ago by roed

comment:2 Changed 9 years ago by roed

  • Authors set to David Roe
  • Cc wstein cremona added
  • Status changed from new to needs_review

Ready for review. You can find a worksheet for testing at It's all using the old code, but you can see some of the problems. Compare with the new code by downloading your own copy and running locally. This would be nicer if we had branches on sagenb already....

comment:3 Changed 9 years ago by roed

  • Cc kedlaya added

Changed 9 years ago by roed

comment:4 Changed 9 years ago by kedlaya

  • Reviewers set to Kiran Kedlaya
  • Status changed from needs_review to positive_review

Patchbot says all tests pass when applied against 5.0beta3, and looks good otherwise. Positive review.

comment:5 Changed 9 years ago by jdemeyer

I'm assuming both patches have to be applied...?

comment:6 Changed 9 years ago by roed


comment:7 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.0.beta14
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.