#4120: [with new patch, needs work] New code for binary quadratic forms
-----------------------------+----------------------------------------------
Reporter: justin | Owner: justin
Type: enhancement | Status: new
Priority: major | Milestone: sage-3.4.1
Component: quadratic forms | Keywords:
-----------------------------+----------------------------------------------
Changes (by tornaria):
* cc: torna...@… (added)
Comment:
I've reviewed the code from the last patch as submitted by John Cremona
(sage-trac4120.new.patch). It applies cleanly on 3.3 and also on top of
the first two patches in #4470.
Below are some comments about the code in {{{binary_qf.py}}}. I didn't
make a difference between old code and code in the patch, since most of
the code is in the patch, anyway.
- Constructor:
- BinaryQF([1,2,3], 4, 5) should raise an error
- I would like to suggest an additional constructor:
{{{
sage: BinaryQF(2, 1, disc = -23)
2*x^2 + x*y + 3*y^2
}}}
this is handy when the discriminant is fixed and one knows the first
two coefficients
of a form
- {{{__repr__}}}:
I don't like the fact that a quadratic form is represented by a
polynomial, may lead
to potential confusion. What about something like:
"{{{Binary quadratic form over Integer Ring with coefficients [a, b,
c]}}}"
?
- polynomial:
- the variables for the polynomial are hardcoded... 'x' and 'y'... not
very important (I rather not have a "polynomial" function... I'd replace
it by a {{{__call__}}} function which works for elements in any ring, then
one can call e.g. {{{Q(x,y)}}} where x and y are in {{{ZZ['x,y']}}}, etc.
- action by matrices:
- Q * M is a left action --> more natural to be right action!!
I.e. right now
{{{
sage: Q = BinaryQF(4,-4,15)
sage: M = matrix(ZZ, 2, [1, 1, 0, 1])
sage: M1 = matrix(ZZ, 2, [1, 0, 1, 1])
sage: Q * M * M1 == Q * (M * M1)
False
sage: Q * M * M1 == Q * (M1 * M)
True
}}}
- I like the notation {{{Q(M)}}} for the action of matrices -- this is
consitent with the
notation for general quadratic forms and representation by vectors or
sublattices (#4470)
[[BR]]
Maybe * should be reserved for composition?
- is_normal: he doctest doesn't explain what it is
- is_equivalent
- IMO should return True or False
- have extra parameter to request transformation matrix
- needs more doctests (in particular indefinite, etc)
- sage: Q * Q.is_equivalent(Q1)[1].transpose() == Q1 /// should be True
this is just an issue with the action of matrices being left action
- for indefinite: should not compute the cycle for every form!!
instead, compute {{{self * other^(-1)}}} (in the class group), and
check if it is in the
principal cycle, which should of course be cached once for each
discriminant. This is
helpful since one will probably use many forms of the same
discriminant together.
[[BR]]
Not sure about how to do memory management though: it'd be nice if
every indefinite
form of discriminant D holds a reference to the principal cycle of
discriminant D, so
the cycle is deleted when the last indefinite form of discriminant D
is deleted ???
[[BR]]
ALSO: IMO the caching of the cycle should be done by the function
Cycle() itself, not by
is_equivalent.
[[BR]]
Moreover, the cycle needs to cache the transformation matrix as well,
so we can
actually figure out the correct transformation matrix.
- matrix: should be the Hessian for consistency with the rest of the code
???
the advantage is that it makes the matrix over ZZ (with even diagonal)
- is_zero: should not need a gcd to decide if it is 0
- s and ss: internal, should be prepended with {{{__}}} ???
- class number computation should use pari
- implement conversions between pari <--> sage for BinaryQF and Qfb
maybe try to wrap around pari functionality as much as possible, for
speed ??? (both
runtime and development!!) E.g. composition, etc.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/4120#comment:16>
Sage <http://sagemath.org/>
Sage - Open Source Mathematical Software: Building the Car Instead of
Reinventing the Wheel
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en
-~----------~----~----~----~------~----~------~--~---
