#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: |
Description
I noticed the following problem with sha().an()
sage: E=EllipticCurve([1215*1216,0]) # non-minimal model sage: E.sha().an() 6.00000000000000 sage: E.minimal_model().sha().an() 1.00000000000000
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() 0.106360349280819 sage: E.minimal_model().period_lattice().omega() 0.638162095684913
It's probably unfair to call this a bug, but it could definitely lead people astray.
Attachments (1)
Change History (9)
comment:1 Changed 12 years ago by
- Cc wuthrich added
- Report Upstream changed from Reported upstream. Developers acknowledge bug. to N/A
comment:2 Changed 12 years ago by
- 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
- Status changed from needs_review to positive_review
comment:4 Changed 12 years ago by
- Reviewers set to Aly Deines
comment:5 Changed 12 years ago by
- Merged in set to sage-4.6.1.alpha1
comment:6 Changed 12 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
comment:7 Changed 11 years ago by
- Reviewers changed from Aly Deines to Alyson Deines
comment:8 Changed 11 years ago by
- Reviewers changed from Alyson Deines to Aly Deines
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...