Opened 12 years ago

Closed 12 years ago

#8319 closed defect (fixed)

elliptic curve canonical height bug for non-minimal models

Reported by: cremona Owned by: cremona
Priority: major Milestone: sage-4.3.4
Component: elliptic curves Keywords: canonical height
Cc: Merged in: sage-4.3.4.alpha0
Authors: John Cremona Reviewers: Chris Wuthrich
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges


For canonical heights of points on elliptic curves defined over QQ, we call the pari function ellheight(), BUT that function only gives the correct value for global minimal models! (At primes where the model is not minimal the local component is wrong).

Here is an example to show this:

sage: E = EllipticCurve([-5580472329446114952805505804593498080000,
....:           -157339733785368110382973689903536054787700497223306368000000])
sage: P3=E([204885147732879546487576840131729064308289385547094673627174585676211859152978311600/23625501907057948132262217188983681204856907657753178415430361,92736254270288706000052529616433462503893110244786566575440613880920567197984949809570153263207165494624214991751142500000000/114834283590033957142081201488956527887145361353994063932282392800295014255070987824900081891])
sage: P3.height()
sage: 4*(P3.height())-(2*P3).height() # should == 0

while on the minimal model:

sage: Emin = E.minimal_model()
sage: urst = E.isomorphism_to(Emin)
sage: 4*urst(P3).height()-urst(2*P3).height()

The cure is to compute the minimal model and transfer the point there before computing the height, as illustrated above. (Of course, pari could do that too, but this behaviour has been tolerated by pari users for many years!)

Attachments (1)

trac_8319-heights.patch (2.0 KB) - added by cremona 12 years ago.
Applies to 4.3.3

Download all attachments as: .zip

Change History (4)

Changed 12 years ago by cremona

Applies to 4.3.3

comment:1 Changed 12 years ago by cremona

  • Authors set to John Cremona
  • Milestone set to sage-4.3.4
  • Status changed from new to needs_review

The patch implements the cure. I did not bother putting in a test for minimality of the original model, since all that does is to compute the minimal model anyway and compare! Also, the minimal model is cached, so that would only be done once. We do not cache the isomorphism to the minimal model, but that is cheap to compute.

comment:2 Changed 12 years ago by wuthrich

  • Reviewers set to Chris Wuthrich
  • Status changed from needs_review to positive_review

Looks fine to me. All tests pass.

comment:3 Changed 12 years ago by mvngu

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