Opened 10 years ago
Closed 10 years ago
#5837 closed defect (fixed)
[with patch, needs review] bug in rational_diagonal_form() from QuadraticForm class
Reported by: | LBerlioz | Owned by: | LBerlioz |
---|---|---|---|
Priority: | major | Milestone: | sage-4.0 |
Component: | quadratic forms | Keywords: | QuadraticForm diagonal |
Cc: | tornaria | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
The following returns a non-diagonal QuadraticForm?:
sage: Q=QuadraticForm(2*A) sage: Q.rational_diagonal_form() Quadratic form in 3 variables over Rational Field with coefficients: [ -3 -32 5184 ] [ * -81 26240 ] [ * * -2125111 ]
This method works only when the matrix has a diagonal of only ones.
Attachments (2)
Change History (8)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Summary changed from bug in rational_diagonal_form() from QuadraticForm class to [with patch, needs review] bug in rational_diagonal_form() from QuadraticForm class
comment:2 Changed 10 years ago by
- Milestone set to sage-3.4.1
Changed 10 years ago by
comment:3 Changed 10 years ago by
- Cc tornaria added
- Milestone changed from sage-3.4.1 to sage-3.4.2
- Priority changed from minor to major
comment:4 Changed 10 years ago by
Some issues in your patch:
- it's not a well formed patch (it's missing the filename to patch)
- some lines look badly indented, the
i=0
in the first line doesn't change the meaning, but the firstelse
is paired with afor
, not sure if that's what you actually mean. - 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. - 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.
comment:5 Changed 10 years ago by
This issue is fixed in the doctest patch in #5954.
comment:6 Changed 10 years ago by
- Milestone changed from sage-4.0.1 to sage-4.0
- Resolution set to fixed
- Status changed from new to closed
Note: See
TracTickets for help on using
tickets.
Hi Luis,
at this stage only blockers will be fixed in 3.4.1, so this ticket is getting bounced to 3.4.2. Since this bug produces mathematically incorrect answers this is also certainly not minor ;)
You should also attach a patch with a doctest that demonstrates that the problem has been fixed. Let me know if you have any questions.
Gonzalo: If you find the time can you review this?
Cheers,
Michael