Extra doctests for indefinite binary quadratic forms

[sagetrac-aliashamieh]

Extra doctests for indefinite binary quadratic forms

CC: @ncalexan

Component: quadratic forms

Keywords: binary quadratic forms

Author: Alia Hamieh

Branch/Commit: 9af692e

Reviewer: John Cremona, Nick Alexander, Simon Brandhorst

Issue created by migration from https://trac.sagemath.org/ticket/6106

[sagetrac-aliashamieh]

Attachment: 6106binaryQuadratic.patch.gz

Attachment: 6106binaryQuadraticPT2.patch.gz

added missing doctests

[loefflerd]

comment:1

Am changing this from "need review" to "needs review" so it shows up in the standard queries

[JohnCremona]

comment:2

Review

The patch applied fine to 4.0.1, and it provides some functions which look useful.

I think that not all new new functions have doctests. Why are the new functions not implemented as member functions of the BinaryQF class? That would be much better; as it is anyone using them has to import them explicitly. So I suggest that they are converted to be member functions (which is trivial to do).

Secondly, the use of the python math functions sqrt and floor is not a good idea, since the coefficients of the form will be Sage integers. For example:

sage: from sage.quadratic_forms.binary_qf import normalized_indefinite_form
sage: f = BinaryQF([10^100,10^500,10^200])
sage: f.discriminant()>0
True
sage: normalized_indefinite_form(f)
---------------------------------------------------------------------------
OverflowError                             Traceback (most recent call last)

/home/john/.sage/temp/ubuntu/9768/_home_john__sage_init_sage_0.py in <module>()

/home/john/sage-4.0.1/local/lib/python2.5/site-packages/sage/quadratic_forms/binary_qf.pyc in normalized_indefinite_form(f)
    486         raise ValueError, "This only works for indefinite forms"
    487     
--> 488     if sqrt(delta) <= abs(a):
    489         s=(a/abs(a))*floor(-1*b/(2*abs(a)))
    490     else:

OverflowError: math range error

For example, the line if sqrt(delta) <= abs(a): could be replaced by if delta <= a**2: .

The docstring for this function should also say what the definition of "normalised" is.

I also slightly amended the ticket's title to give a better description.

[ncalexan]

comment:3

Alia and I are going to work on this in the next few days.

[ncalexan]

Reviewer: John Cremona, Nick Alexander

[ncalexan]

Author: Alia Hamieh

[sagetrac-aliashamieh]

Attachment: 6106BinaryQF.patch.gz

[with patch, needs review] Additional functions for Indefinite Binary Quadratic Forms

[pjbruin]

comment:6

This ticket seems to implement the same functionality as (part of) #4120, which has been inactive for a very long time, like this ticket.

[pjbruin]

comment:9

Replying to @pjbruin:

This ticket seems to implement the same functionality as (part of) #4120, which has been inactive for a very long time, like this ticket.

Now that #4120 has been merged, it would be good to check if this ticket still adds new functionality. If not, we could also just add the examples from this ticket.

[simonbrandhorst]

comment:10

This ticket does not add anything new.

[pjbruin]

comment:11

Replying to @simonbrandhorst:

This ticket does not add anything new.

OK. It might still be useful to add the doctests from this ticket; what do you think?

[simonbrandhorst]

comment:12

If I recall correctly the reduction of an indefinite form is not unique but instead we get a cycle of forms. So we can add some doctests. But the results may be different.

Note that we messed up a little for indefinite forms of square determinant #25861.

[pjbruin]

Commit: 9af692e

[pjbruin]

Branch: u/pbruin/6106-BinaryQF_examples

[pjbruin]

comment:13

Here are some doctests from the patches. I checked that they are mathematically correct. The docstring for reduced_form() did not have any examples with transformation=True yet; now it does.

[pjbruin]

Changed reviewer from John Cremona, Nick Alexander to John Cremona, Nick Alexander, Simon Brandhorst, Peter Bruin

[simonbrandhorst]

Changed commit from 9af692e to none

[simonbrandhorst]

comment:14

doc builds.

[simonbrandhorst]

Changed branch from u/pbruin/6106-BinaryQF_examples to none

[simonbrandhorst]

Changed reviewer from John Cremona, Nick Alexander, Simon Brandhorst, Peter Bruin to John Cremona, Nick Alexander, Simon Brandhorst

[pjbruin]

Commit: 9af692e

[pjbruin]

comment:16

Replying to @simonbrandhorst:

doc builds.

From your other changes, it doesn't look like you really intended to delete the branch and change the milestone to sage-duplicate/invalid/wontfix...

[pjbruin]

Branch: u/pbruin/6106-BinaryQF_examples

[simonbrandhorst]

comment:17

Ooops indeed. Thank you.

[vbraun]

Changed branch from u/pbruin/6106-BinaryQF_examples to 9af692e