Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#5766 closed defect (fixed)

[with patch; positive review] improve coverage of structure/formal_sum.py

Reported by: was Owned by: somebody
Priority: major Milestone: sage-4.0.1
Component: basic arithmetic Keywords:
Cc: Merged in: 4.0.1.rc1
Authors: William Stein, Alex Ghitza Reviewers: John Cremona, Mike Hansen
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description


Attachments (4)

trac_5766.patch (12.6 KB) - added by was 11 years ago.
trac_5766-doctest_fix.patch (1.0 KB) - added by was 11 years ago.
trac_5766-part3.patch (1.1 KB) - added by was 11 years ago.
trac_5766-rebased.patch (13.1 KB) - added by AlexGhitza 11 years ago.
apply instead of the other patches

Download all attachments as: .zip

Change History (17)

comment:1 Changed 11 years ago by was

BUGS FOUND:

  1. The reduce option to formal sum is totally ignored.
    sage: FormalSum([(1,2/3), (3,2/3), (-5, 7)], reduce=False)
    4*2/3 - 5*7
    
  1. Latexing formal sums is completely broken (I think this is actually #5707):
    sage: latex(FormalSum([(1,2), (5, 8/9), (-3, 7)]))
    5\frac{8}{9}2 - 37
    

Changed 11 years ago by was

comment:2 Changed 11 years ago by was

  • Summary changed from improve coverage of structure/formal_sum.py to [with patch; needs review] improve coverage of structure/formal_sum.py

comment:3 Changed 11 years ago by ncalexan

  • Summary changed from [with patch; needs review] improve coverage of structure/formal_sum.py to [with patch; with positive review] improve coverage of structure/formal_sum.py

There's an indirect doctest missing from nonzero and a typoed indirect doctest on len, but other than that this looks good.

comment:4 Changed 11 years ago by mabshoff

  • Summary changed from [with patch; with positive review] improve coverage of structure/formal_sum.py to [with patch; needs work] improve coverage of structure/formal_sum.py

Mhh, ther seems to be a 32 vs. 64 bit issue here:

sage -t -long "devel/sage/sage/structure/formal_sum.py"     
**********************************************************************
File "/scratch/mabshoff/sage-3.4.1.rc3/devel/sage/sage/structure/formal_sum.py", line 333:
    sage: a
Expected:
    2/3 - 3*4/5 + 7*2
Got:
    7*2 + 2/3 - 3*4/5
**********************************************************************
File "/scratch/mabshoff/sage-3.4.1.rc3/devel/sage/sage/structure/formal_sum.py", line 335:
    sage: a._repr_()
Expected:
    '2/3 - 3*4/5 + 7*2'
Got:
    '7*2 + 2/3 - 3*4/5'
**********************************************************************

Cheers,

Michael

Changed 11 years ago by was

comment:5 Changed 11 years ago by was

  • Summary changed from [with patch; needs work] improve coverage of structure/formal_sum.py to [with patch; needs review] improve coverage of structure/formal_sum.py

comment:6 Changed 11 years ago by cremona

  • Summary changed from [with patch; needs review] improve coverage of structure/formal_sum.py to [with patch; with review, needs work] improve coverage of structure/formal_sum.py

Sorry, but on my 32-bit laptop I get

**********************************************************************
File "/home/john/sage-3.4.2.alpha0/devel/sage-tests/sage/structure/formal_sum.py", line 333:
    sage: a
Expected:
    2/3 - 3*4/5 + 7*2                       -- comparing Mod(2,3) and rationals ill-defined
Got:
    7*2 + 2/3 - 3*4/5
**********************************************************************
File "/home/john/sage-3.4.2.alpha0/devel/sage-tests/sage/structure/formal_sum.py", line 336:
    sage: a._repr_()
Expected:
    '2/3 - 3*4/5 + 7*2'                    
Got:
    '7*2 + 2/3 - 3*4/5'
**********************************************************************

Perhaps all formal sums should be sorted? As part of the reduce() method (which should then be called after creation)?

comment:7 Changed 11 years ago by was

  • Summary changed from [with patch; with review, needs work] improve coverage of structure/formal_sum.py to [with patch; needs review] improve coverage of structure/formal_sum.py

Perhaps all formal sums should be sorted? As part of the reduce() method (which should then be called after creation)?

They are sorted. That's why the doctests failed for you -- because sort is not well defined as indicated in the comment.

Changed 11 years ago by was

comment:8 Changed 11 years ago by cremona

  • Summary changed from [with patch; needs review] improve coverage of structure/formal_sum.py to [with patch; positive review] improve coverage of structure/formal_sum.py

OK with all three patches the tests pass. It's a pity we cannot make it deterministic but the only thing I could think of was sorting on the strong representations (of the base of each pair) and that would take too long I suppose.

comment:9 Changed 11 years ago by mabshoff

  • Milestone changed from sage-4.0 to sage-3.4.2

With all three patch applied I am seeing the following doctest failure:

sage -t -long "devel/sage/sage/misc/latex.py"               
**********************************************************************
File "/scratch/mabshoff/sage-3.4.2.rc0/devel/sage/sage/misc/latex.py", line 942:
    sage: repr_lincomb([1,5,-3],[2,8/9,7])
Expected:
    '2*1 + 8/9*5 + 7*-3'
Got:
    '21 + \\frac{8}{9}5 + 7-3'
**********************************************************************

Since this seems to be the correct LaTeX representation I am fixing this failure.

Cheers,

Michael

comment:10 Changed 11 years ago by mabshoff

  • Milestone changed from sage-3.4.2 to sage-4.0
  • Summary changed from [with patch; positive review] improve coverage of structure/formal_sum.py to [with patch; needs work] improve coverage of structure/formal_sum.py

Actually, the above seems broken, i.e. notice the missing * - so 'needs work'.

Cheers,

Michaek

comment:11 Changed 11 years ago by AlexGhitza

  • Summary changed from [with patch; needs work] improve coverage of structure/formal_sum.py to [with patch; needs review] improve coverage of structure/formal_sum.py

I have folded William's patches and rebased them against 4.0.alpha0.

I made an attempt to fix the latex-ing, see the patch.

Changed 11 years ago by AlexGhitza

apply instead of the other patches

comment:12 Changed 11 years ago by mhansen

  • Resolution set to fixed
  • Status changed from new to closed
  • Summary changed from [with patch; needs review] improve coverage of structure/formal_sum.py to [with patch; positive review] improve coverage of structure/formal_sum.py

Looks good to me.

Merged trac_5766-rebased.patch in 4.0.1.rc1.

comment:13 Changed 11 years ago by mhansen

  • Authors set to William Stein, Alex Ghitza
  • Merged in set to 4.0.1.rc1
  • Reviewers set to John Cremona, Mike Hansen
Note: See TracTickets for help on using tickets.