#5766 closed defect (fixed)
[with patch; positive review] improve coverage of structure/formal_sum.py
Reported by: | William Stein | 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: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Attachments (4)
Change History (17)
comment:1 Changed 14 years ago by
Changed 14 years ago by
Attachment: | trac_5766.patch added |
---|
comment:2 Changed 14 years ago by
Summary: | improve coverage of structure/formal_sum.py → [with patch; needs review] improve coverage of structure/formal_sum.py |
---|
comment:3 Changed 14 years ago by
Summary: | [with patch; needs review] improve coverage of structure/formal_sum.py → [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 14 years ago by
Summary: | [with patch; with positive review] improve coverage of structure/formal_sum.py → [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 14 years ago by
Attachment: | trac_5766-doctest_fix.patch added |
---|
comment:5 Changed 14 years ago by
Summary: | [with patch; needs work] improve coverage of structure/formal_sum.py → [with patch; needs review] improve coverage of structure/formal_sum.py |
---|
comment:6 Changed 14 years ago by
Summary: | [with patch; needs review] improve coverage of structure/formal_sum.py → [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 14 years ago by
Summary: | [with patch; with review, needs work] improve coverage of structure/formal_sum.py → [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 14 years ago by
Attachment: | trac_5766-part3.patch added |
---|
comment:8 Changed 14 years ago by
Summary: | [with patch; needs review] improve coverage of structure/formal_sum.py → [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 14 years ago by
Milestone: | sage-4.0 → 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 14 years ago by
Milestone: | sage-3.4.2 → sage-4.0 |
---|---|
Summary: | [with patch; positive review] improve coverage of structure/formal_sum.py → [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 14 years ago by
Summary: | [with patch; needs work] improve coverage of structure/formal_sum.py → [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 14 years ago by
Attachment: | trac_5766-rebased.patch added |
---|
apply instead of the other patches
comment:12 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Summary: | [with patch; needs review] improve coverage of structure/formal_sum.py → [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 14 years ago by
Authors: | → William Stein, Alex Ghitza |
---|---|
Merged in: | → 4.0.1.rc1 |
Reviewers: | → John Cremona, Mike Hansen |
BUGS FOUND: