Ticket #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 | Work issues: | |
| Report Upstream: | N/A | Reviewers: | David Loeffler, Robert Bradshaw |
| Authors: | John Cremona | Merged in: | sage-4.5.2.alpha0 |
| Dependencies: | Stopgaps: |
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
-
trac_9372-regulator.patch
(12.4 KB) -
added by cremona 3 years ago.
- Applies to 4.4.4
-
trac_9372-regulator.2.patch
(12.5 KB) -
added by aly.deines 3 years ago.
- doctest fixed -- replaces previous patch
Change History
comment:1 Changed 3 years ago by cremona
- Owner changed from AlexGhitza to cremona
- Component changed from algebra to elliptic curves
comment:2 Changed 3 years ago by cremona
- Status changed from new to needs_review
- Authors set to John Cremona
The patch moves the two functions height_pairing_matrix and regulator_of_points, and adds doctests over number fields.
comment:3 Changed 3 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 3 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 3 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 3 years ago by aly.deines
-
attachment
trac_9372-regulator.2.patch
added
doctest fixed -- replaces previous patch
comment:6 Changed 3 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:8 Changed 3 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!).
