Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#5954 closed enhancement (fixed)

[with patch, positive review] Added documentation/doctests for all quadratic form genus symbol routines

Reported by: jonhanke Owned by: tbd
Priority: major Milestone: sage-4.0
Component: quadratic forms Keywords: QuadraticForm
Cc: tornaria, mabshoff, wstein, cremona Merged in: 4.0.rc0
Authors: Jon Hanke Reviewers: Gonzalo Tornaria
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

This patch documents and brings doctest coverage to 100% for all routines in:

quadratic_forms/quadratic_form__genus.py
quadratic_forms/genera/genus.py

It also rewrote a few signature routines and the rational_diagonal_form() routine which was causing the bug reported in Ticket #5837:

sage: Q2=QuadraticForm(ZZ,3,[ -3,2,0 , 3,-2 , 5 ])
sage: Q2.rational_diagonal_form()

Quadratic form in 3 variables over Rational Field with coefficients: 
[ -3 0 0 ]
[ * 10/3 0 ]
[ * * 47/10 ]

Attachments (1)

patch-1__QF_genus_symbols__3.4.1.patch (57.3 KB) - added by jonhanke 9 years ago.

Download all attachments as: .zip

Change History (8)

Changed 9 years ago by jonhanke

comment:1 Changed 9 years ago by was

I just skimmed through patch-1QF_genus_symbols3.4.1.patch and I'm very happy with what I'm seeing! I did order the Repo Man movie, in case I needed lessons in how to rip out all the quadratic forms code. It looks like I won't :-)

-- William

comment:2 Changed 9 years ago by mabshoff

  • Cc cremona added

Hehe, nice comment William. There is at least one typo in the docstring:

        doubling conventions strraight throughout!  This is especially 

I have skimmed the rest and did not see anything obvious. The renaming could introduce some trouble, so adding deprecation messages for the old names might be a good idea. We will see what the real reviewer will say.

I am CCing John also to see if he can slip it into their review schedule. John: in case you are busy feel free to ignore this. :)

Cheers,

Michael

comment:3 Changed 9 years ago by tornaria

Looks good to me. The renamings mentioned by mabshoff are:

  • signature_of_matrix renamed to signature_pair_of_matrix
  • is_even renamed to is_even_matrix
  • trace_diag renamed to trace_diag_mod_8
  • is_trivial_symbol removed

Of these the first three are internal, not exported in {{{sage.quadratic_forms.all}}. The renaming aids to readability. The fourth one was actually exported, but apparently it was not clear what it does (the comment reads "Removed because it was unused and undocumented!"). Jon may want to add a comment about it here.

Other than that, I will be giving positive review together with the whole series of QF doctest patches summarized in #6040, since coverage goes up to 100% and all doctest pass after a few trivial bugfixes.

Incidentally, this patch fixes the issue in #5837, so that one should be closed as this patch gets merged.

comment:4 Changed 9 years ago by mabshoff

  • Summary changed from Added documentation/doctests for all quadratic form genus symbol routines to [with patch, positive review] Added documentation/doctests for all quadratic form genus symbol routines

Mark the ticket properly as Gonzalo reviewed it.

Cheers,

Michael

comment:5 Changed 9 years ago by tornaria

I'm ok with the positive review.

comment:6 Changed 9 years ago by mabshoff

  • Milestone changed from sage-4.0.1 to sage-4.0
  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 4.0.rc0.

Cheers,

Michael

comment:7 Changed 8 years ago by davidloeffler

  • Authors set to Jon Hanke
  • Component changed from algebra to quadratic forms
  • Merged in set to 4.0.rc0
  • Reviewers set to Gonzalo Tornaria
Note: See TracTickets for help on using tickets.