Opened 12 years ago

Closed 12 years ago

#9372 closed enhancement (fixed)

implement regulator function for elliptic curves over number fields

Reported by: cremona Owned by: cremona
Priority: major Milestone: sage-4.5.2
Component: elliptic curves Keywords:
Cc: robertwb Merged in: sage-4.5.2.alpha0
Authors: John Cremona Reviewers: David Loeffler, Robert Bradshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Now that we have canonical heights over number fields, the regulator_of_points code can be moved up from ell_rational_field to ell_number_field.

Attachments (2)

trac_9372-regulator.patch (12.4 KB) - added by cremona 12 years ago.
Applies to 4.4.4
trac_9372-regulator.2.patch (12.5 KB) - added by aly.deines 12 years ago.
doctest fixed -- replaces previous patch

Download all attachments as: .zip

Change History (11)

comment:1 Changed 12 years ago by cremona

  • Component changed from algebra to elliptic curves
  • Owner changed from AlexGhitza to cremona

Changed 12 years ago by cremona

Applies to 4.4.4

comment:2 Changed 12 years ago by cremona

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

The patch moves the two functions height_pairing_matrix and regulator_of_points, and adds doctests over number fields.

comment:3 Changed 12 years ago by davidloeffler

  • Status changed from needs_review to needs_work

I'm getting a couple of doctest failures -- something is off by a sign in the height_pairing_matrix code:

sage -t -long ell_number_field.py
**********************************************************************
File "/storage/masiao/sage-4.4.4/devel/sage-reviewing/sage/schemes/elliptic_curves/ell_number_field.py", line 308:
    sage: E.height_pairing_matrix([P,Q])
Expected:
    [ 0.686667083305587 -0.268478098806726]
    [-0.268478098806726  0.327000773651605]
Got:
    [0.686667083305587 0.268478098806726]
    [0.268478098806726 0.327000773651605]
**********************************************************************
File "/storage/masiao/sage-4.4.4/devel/sage-reviewing/sage/schemes/elliptic_curves/ell_number_field.py", line 317:
    sage: EK.height_pairing_matrix([EK(P),EK(Q)])
Expected:
    [ 0.686667083305586 -0.268478098806726]
    [-0.268478098806726  0.327000773651605]
Got:
    [0.686667083305586 0.268478098806726]
    [0.268478098806726 0.327000773651605]
**********************************************************************
1 items had failures:
   2 of  23 in __main__.example_4
***Test Failed*** 2 failures.

Also, a very tiny quibble: the second argument "precision" to height_pairing_matrix is missing its bullet point in the docstring.

comment:4 Changed 12 years ago by cremona

Probably I used E.gens() which is a bad idea in odctests, better to enter the points manually.

No time to fix now, about to leave SD22 for home.... but thanks all the same! Put this patch up from a coffee shop last night just before it closed (about a dozen Sagers being chased out!)

comment:5 Changed 12 years ago by aly.deines

@cremona: You did you E.gens(). I get:

sage: E = EllipticCurve('389a1')
sage: E.gens()
[(-1 : 1 : 1), (0 : -1 : 1)]

Should I change the doc test to the following?

sage: E = EllipticCurve('389a1')
sage: P,Q = E.point([-1,1,1]),E.point([0,-1,1])
sage: E.height_pairing_matrix([P,Q])
[0.686667083305587 0.268478098806726]
[0.268478098806726 0.327000773651605]

}}}

Changed 12 years ago by aly.deines

doctest fixed -- replaces previous patch

comment:6 Changed 12 years ago by aly.deines

  • Status changed from needs_work to needs_review

If the only problem was that the doctest called E.gens(), this fixes those doctests and you can give this a positive review.

comment:7 Changed 12 years ago by robertwb

  • Status changed from needs_review to positive_review

comment:8 Changed 12 years ago by cremona

Thanks to all for sorting that out. I should have known better. Even for curves in the database, it is not safe to use gens() since unless you have the larger database installed the gens are computed on the fly and are not unique. (And doctests definitely should not assume an optional spkg is installed!).

comment:9 Changed 12 years ago by mpatel

  • Merged in set to sage-4.5.2.alpha0
  • Resolution set to fixed
  • Reviewers set to David Loeffler, Robert Bradshaw
  • Status changed from positive_review to closed

I'm updating the Reviewer(s) field. Please correct me if I'm wrong.

Note: See TracTickets for help on using tickets.