Opened 9 years ago

Closed 8 years ago

#13953 closed defect (fixed)

(non)archimedian_local_height of a torsion points always gives 0

Reported by: pbruin Owned by: cremona
Priority: major Milestone: sage-5.10
Component: elliptic curves Keywords: local heights
Cc: Merged in: sage-5.10.beta4
Authors: Peter Bruin Reviewers: Chris Wuthrich
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12509 Stopgaps:

Status badges

Description (last modified by pbruin)

For torsion points of elliptic curves over number fields, nonarchimedian_local_height incorrectly returns 0, and archimedian_local_height raises an error:

sage: K.<i> = QuadraticField(-1)
sage: E = EllipticCurve([0, 0, 0, K(1), 0])
sage: P = E(i, 0)
sage: P.nonarchimedian_local_height()
0
sage: P.archimedian_local_height()
NameError: global name 'QQ' is not defined

The correct behaviour (for the normalisation used in Sage) is

sage: P.nonarchimedian_local_height()
-1/2*log(2)
sage: P.archimedian_local_height()
0.346573590279973

Note: the same would happen for rational points on elliptic curves over Q, but computing local heights over Q is broken (see #13951).

Apply: trac13953-local_heights_torsion.patch

Attachments (1)

trac13953-local_heights_torsion.patch (1.9 KB) - added by pbruin 8 years ago.
based on 5.9 + three patches of #12509

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 years ago by pbruin

  • Authors set to Peter Bruin
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 follow-up: Changed 9 years ago by cremona

2 quick questions: (1) is this patch based on #12509? (2) is your fix just to remove the wrong quick exit for torsion points (as appears)?

comment:3 in reply to: ↑ 2 ; follow-up: Changed 9 years ago by pbruin

Replying to cremona:

(1) is this patch based on #12509?

No, but I have another branch where I made the same patch based on #12509; would it be better to upload that one?

(2) is your fix just to remove the wrong quick exit for torsion points (as appears)?

Yes, the rest of the patch just adds a doctest.

comment:4 in reply to: ↑ 3 Changed 9 years ago by cremona

Replying to pbruin:

Replying to cremona:

(1) is this patch based on #12509?

No, but I have another branch where I made the same patch based on #12509; would it be better to upload that one?

I think it would: you can add that ticket as a dependency of this one. Then they should get merged together.

comment:5 Changed 9 years ago by pbruin

  • Dependencies set to #12509

comment:6 Changed 8 years ago by wuthrich

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

The patch applies cleanly to 5.9 after #12509. The tests pass and it does what it should do.

comment:7 Changed 8 years ago by wuthrich

  • Status changed from positive_review to needs_work

Changed 8 years ago by pbruin

based on 5.9 + three patches of #12509

comment:8 Changed 8 years ago by pbruin

  • Status changed from needs_work to needs_review

comment:9 Changed 8 years ago by wuthrich

  • Status changed from needs_review to positive_review

Thanks for rebasing it. (Sorry for not commenting, the browser in my office decided to log me out permanently so I could not post a comment on what needs to be done).

I tested it again and it is again all fine.

comment:10 Changed 8 years ago by pbruin

Thanks! And no problem, it was clear what had to be done.

comment:11 Changed 8 years ago by jdemeyer

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