#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)
Change History (8)
Changed 9 years ago by
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
- 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
Looks good to me. The renamings mentioned by mabshoff are:
signature_of_matrix
renamed tosignature_pair_of_matrix
is_even
renamed tois_even_matrix
trace_diag
renamed totrace_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
- 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
I'm ok with the positive review.
comment:6 Changed 9 years ago by
- 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 9 years ago by
- Component changed from algebra to quadratic forms
- Merged in set to 4.0.rc0
- Reviewers set to Gonzalo Tornaria
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 :-)