Opened 14 years ago

Closed 14 years ago

Last modified 13 years ago

#4388 closed defect (fixed)

[with patch, with positive review] elliptic curves: basis_matrix command totally broken

Reported by: William Stein Owned by: William Stein
Priority: major Milestone: sage-3.2
Component: number theory Keywords:
Cc: Merged in: 3.2.alpha3
Authors: John Cremona Reviewers: David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

sage: EllipticCurve('11a').period_lattice().basis_matrix()
Traceback (most recent call last):
...
TypeError: Unable to coerce 0.634604652139777 + 1.45881661693850*I (<type 'sage.rings.complex_number.ComplexNumber'>) to Rational

Attachments (1)

sage-trac4388.patch (2.2 KB) - added by John Cremona 14 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 14 years ago by John Cremona

Comment: I noticed this when I reworked the whole of period_lattice.py recently. But the function basis_matrix only exists because PeriodLattice_ell derives from PeriodLattice? and hence from FreeModule_generic_pid. But I don't think it makes a lot of sense to ask for a basis matrix in a case like this, when the thing is a Z-module but it does not sit in an ambient Q-vector space.

If people agree, we should at least add the function but have it raise a sensible error.

comment:2 Changed 14 years ago by William Stein

But I really wanted basis_matrix(), since I wanted to compute the determinant of the basis matrix in order to find the volume of the period lattice.

There is no volume method. That would also be nice.

I think at least mathematically the idea of "basis matrix" makes sense, and I was happy it was there (except that it is broken).

comment:3 in reply to:  2 Changed 14 years ago by John Cremona

Replying to was:

But I really wanted basis_matrix(), since I wanted to compute the determinant of the basis matrix in order to find the volume of the period lattice.

There is no volume method. That would also be nice.

It _is_ there: complex_area() (not my choice of name)!

I think at least mathematically the idea of "basis matrix" makes sense, and I was happy it was there (except that it is broken).

You'll have to explain it to me. Do you want the 2x2 matrix of reals consisting of the real and imaginary parts of the period basis? That would be easy to add, like this:

sage: E = EllipticCurve('389a1')
sage: L = E.period_lattice()
sage: M = Matrix([[CC(w).real(), CC(w).imag()] for w in L.basis()]); M

[ 2.49021256085505 0.000000000000000]
[0.000000000000000  1.97173770155165]
sage: M.det()
4.91004599111539
sage: L.complex_area()
4.91004599111539

and

sage: E = EllipticCurve('11a1')
sage: L = E.period_lattice()
sage: M = Matrix([[CC(w).real(), CC(w).imag()] for w in L.basis()]); M

[ 1.26920930427955 0.000000000000000]
[0.634604652139777  1.45881661693850]
sage: M.det()
1.85154362345596
sage: L.complex_area()
1.85154362345596

Changed 14 years ago by John Cremona

Attachment: sage-trac4388.patch added

comment:4 Changed 14 years ago by John Cremona

Summary: elliptic curves: basis_matrix command totally broken[with patch, needs review] elliptic curves: basis_matrix command totally broken

Patch sage-trac4388.patch attached (based on 3.2.alpha1).

comment:5 Changed 14 years ago by David Loeffler

Summary: [with patch, needs review] elliptic curves: basis_matrix command totally broken[with patch, with positive review] elliptic curves: basis_matrix command totally broken

Looks good to me. I agree with was's statement that the concept of a basis matrix makes sense here, and that basis_matrix() should return this rather than an error; patch applies fine in 3.2.alpha1; and all doctests in sage/schemes/elliptic_curves pass.

comment:6 Changed 14 years ago by Michael Abshoff

Resolution: fixed
Status: newclosed

Merged in Sage 3.2.alpha3

comment:7 Changed 13 years ago by David Loeffler

Authors: John Cremona
Merged in: 3.2.alpha3
Reviewers: David Loeffler
Note: See TracTickets for help on using tickets.