#5837: [with patch, needs review] bug in rational_diagonal_form() from
QuadraticForm class
-----------------------------+----------------------------------------------
Reporter: LBerlioz | Owner: LBerlioz
Type: defect | Status: new
Priority: major | Milestone: sage-3.4.2
Component: quadratic forms | Keywords: QuadraticForm diagonal
-----------------------------+----------------------------------------------
Comment(by tornaria):
Some issues in your patch:
a. it's not a well formed patch (it's missing the filename to patch)
b. some lines look badly indented, the {{{i=0}}} in the first line
doesn't change the meaning, but the first {{{else}}} is paired with a
{{{for}}}, not sure if that's what you actually mean.
c. you should add a test case which shows the bug has been fixed. For
instance, add something like
{{{
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 ]
}}}
to the doctest.
d. you are also changing the function to correctly handle quadratic forms
with 0 in the diagonal. You should write a doctest for that case as well.
Personally, I'd avoid the exception and rewrite the code making an
explicit test for {{{Q[i,i]Q=0}}}. This could also helps keeping the more
natural {{{for i in range(n):}}} outer loop, which is easier to read than
the {{{while i < n-1:}}} loop. The loop would just do a different thing
depending on {{{Q[i,i]=0}}} or not. Just a suggestion, since you are
writing the patch, do as you see is more convenient.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/5837#comment:4>
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
-~----------~----~----~----~------~----~------~--~---
