Opened 11 years ago

Closed 11 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)

trac_5837 (1.5 KB) - added by LBerlioz 11 years ago.
trac_5837.patch (1.5 KB) - added by LBerlioz 11 years ago.

Download all attachments as: .zip

Change History (8)

Changed 11 years ago by LBerlioz

comment:1 Changed 11 years ago by LBerlioz

  • 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 11 years ago by LBerlioz

  • Milestone set to sage-3.4.1

Changed 11 years ago by LBerlioz

comment:3 Changed 11 years ago by mabshoff

  • Cc tornaria added
  • Milestone changed from sage-3.4.1 to sage-3.4.2
  • Priority changed from minor to major

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

comment:4 Changed 11 years ago by tornaria

Some issues in your patch:

  1. it's not a well formed patch (it's missing the filename to patch)
  2. 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.
  3. 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.
  4. 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 11 years ago by tornaria

This issue is fixed in the doctest patch in #5954.

comment:6 Changed 11 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

Fixed via #5954, but credit still goes to Luiz for this ticket.

Cheers,

Michael

Note: See TracTickets for help on using tickets.