Project: #4120

There is a (combined) patch in Trac #4120 including my first three patches, and John Cremona's bug fixes and updates. In addition, John has a list of concerns and bugs to be addressed. Gonzalo Tornaria has additional comments and suggestions.

This project addresses (some of) these. Individual comments from John and Gonzalo are given below in three sections: Done, In Progress, and Discussion Needed.

Done

• __repr__: NOT TO BE DONE; Minor problem: BinaryQF(0,0,0)

  • 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: DONE

  • 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: DONE (left, right action; functional right action Q(M))

  • 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) Maybe * should be reserved for composition?

• is_normal: DONE
  • the doctest doesn't explain what it is

• is_zero: DONE

  • should not need a gcd to decide if it is 0

• s and ss: Can't use __ (reserved for Python); this is now moot (changed implementation)

  • internal, should be prepended with __ ???

In Progress

• 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. 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 ???

• ALSO: IMO the caching of the cycle should be done by the function Cycle() itself, not by is_equivalent. 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)
• Finally:

  • 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.

Discussion Needed

• 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.

• is_equivalent:
  • IMO should return True or False
  • have extra parameter to request transformation matrix
  • needs more doctests (in particular indefinite, etc) - DONE
  • Equivalence verification: DONE

        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 - Actually, there is a bug that prevents this from working.

• The various transform function which return a new form Q and a transform T really must satisfy self.T==Q, but they don't. Hence the commented out assertions.
• We must decide whether we are talking about weak or strict equivalence (GL or SL(2,ZZ)). At the moment it is hard to tell which.
• For indefinite forms there are several different notions of "reduced". OK to to stick to one, but we should make this explicit.
• The class number function looks inefficient to me, it should be replaced by the fast code by Skoruppa to list reduced forms (in the definite case at least).