Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#10096 closed defect (fixed)

sha().an() assumes E is minimal.

Reported by: weigandt Owned by: cremona
Priority: major Milestone: sage-4.6.1
Component: elliptic curves Keywords: elliptic curves, sha, real period
Cc: was, rlm, wuthrich Merged in: sage-4.6.1.alpha1
Authors: John Cremona Reviewers: Aly Deines
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges


I noticed the following problem with sha().an()

sage: E=EllipticCurve([1215*1216,0]) # non-minimal model
sage: E.sha().an()
sage: E.minimal_model().sha().an()

It looks like sha().an() assumes that E is minimal. The extra factor of 6 seems to be coming from the real period.

sage: E.period_lattice().omega()
sage: E.minimal_model().period_lattice().omega()

It's probably unfair to call this a bug, but it could definitely lead people astray.

Attachments (1)

trac_10096-sha.patch (10.5 KB) - added by cremona 12 years ago.
Applies to 4.6.rc0

Download all attachments as: .zip

Change History (9)

comment:1 Changed 12 years ago by cremona

  • Cc wuthrich added
  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to N/A

I found the problem here. an() uses the minimal model when it can do the job itself, which is when the rank is at most 1, but when the rank is at least 2 it passes the work to an_numerical(), and that did not use the minimal model.

I am preparing a patch which fixes this. And as I suspect that other related functions also need the minimal model (notably the p-adic one) I am now caching the minimal model and changing lots of occurrences of self.E to self.Emin.

Watch this space...

Changed 12 years ago by cremona

Applies to 4.6.rc0

comment:2 Changed 12 years ago by cremona

  • Authors set to John Cremona
  • Status changed from new to needs_review

The patch fixes this. the minimal model is computed on construction and used in place of the original (when the latter is not minimal). New doctests show that the reported problem has gone away.

It would be helpful if someone could confirm that this is the correct thing to do also for the an_padic() function. The doctests still pass, but I sust[ect that they are all with minimal curves anyway. I was assuming (a) that the value of an_padic() was isomorphism-invariant, and (2) that using a minimal model would be at least as fast, possibly faster, than using a non-minimal model, even if both give the correct answer!

comment:3 Changed 12 years ago by aly.deines

  • Status changed from needs_review to positive_review

comment:4 Changed 12 years ago by jdemeyer

  • Reviewers set to Aly Deines

comment:5 Changed 12 years ago by jdemeyer

  • Merged in set to sage-4.6.1.alpha1

comment:6 Changed 12 years ago by jdemeyer

  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:7 Changed 11 years ago by jdemeyer

  • Reviewers changed from Aly Deines to Alyson Deines

comment:8 Changed 11 years ago by jdemeyer

  • Reviewers changed from Alyson Deines to Aly Deines
Note: See TracTickets for help on using tickets.