Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement regulator function for elliptic curves over number fields #9372

Closed
JohnCremona opened this issue Jun 29, 2010 · 12 comments
Closed

Comments

@JohnCremona
Copy link
Member

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.

CC: @robertwb

Component: elliptic curves

Author: John Cremona

Reviewer: David Loeffler, Robert Bradshaw

Merged: sage-4.5.2.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/9372

@JohnCremona
Copy link
Member Author

Applies to 4.4.4

@JohnCremona
Copy link
Member Author

comment:2

Attachment: trac_9372-regulator.patch.gz

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

@JohnCremona
Copy link
Member Author

Author: John Cremona

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Jun 29, 2010

comment:3

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.

@loefflerd loefflerd mannequin added s: needs work and removed s: needs review labels Jun 29, 2010
@JohnCremona
Copy link
Member Author

comment:4

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!)

@adeines
Copy link
Mannequin

adeines mannequin commented Jun 29, 2010

comment:5

@JohnCremona: 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]

}}}

@adeines
Copy link
Mannequin

adeines mannequin commented Jun 30, 2010

doctest fixed -- replaces previous patch

@adeines
Copy link
Mannequin

adeines mannequin commented Jun 30, 2010

comment:6

Attachment: trac_9372-regulator.2.patch.gz

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

@JohnCremona
Copy link
Member Author

comment:8

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!).

@qed777
Copy link
Mannequin

qed777 mannequin commented Jul 20, 2010

Reviewer: David Loeffler, Robert Bradshaw

@qed777
Copy link
Mannequin

qed777 mannequin commented Jul 20, 2010

Merged: sage-4.5.2.alpha0

@qed777
Copy link
Mannequin

qed777 mannequin commented Jul 20, 2010

comment:9

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

@qed777 qed777 mannequin removed the s: positive review label Jul 20, 2010
@qed777 qed777 mannequin closed this as completed Jul 20, 2010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants