-
-
Notifications
You must be signed in to change notification settings - Fork 419
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
elliptic curves: basis_matrix command totally broken #4388
Comments
comment:1
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
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
Replying to @williamstein:
It
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:
and
|
Attachment: sage-trac4388.patch.gz |
comment:4
Patch sage-trac4388.patch attached (based on 3.2.alpha1). |
comment:5
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
Merged in Sage 3.2.alpha3 |
Merged: 3.2.alpha3 |
Reviewer: David Loeffler |
Author: John Cremona |
Component: number theory
Author: John Cremona
Reviewer: David Loeffler
Merged: 3.2.alpha3
Issue created by migration from https://trac.sagemath.org/ticket/4388
The text was updated successfully, but these errors were encountered: