Opened 15 years ago

Closed 15 years ago

#1805 closed defect (fixed)

[with patch; Positive review] improve doctest coverage for Factorization; fix several critical bugs

Reported by: William Stein Owned by: ncalexan
Priority: major Milestone: sage-2.10.3
Component: doctest coverage Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description


Attachments (6)

trac-1805.patch (22.3 KB) - added by William Stein 15 years ago.
trac-1805-part2.patch (5.7 KB) - added by William Stein 15 years ago.
this part 2 fixes some additional issues, typos, etc., I saw when proofreading my patch.
sage-1805-rebased_2.10.3.patch (25.9 KB) - added by William Stein 15 years ago.
new rebased version
sage-1805-part2-rebased.patch (3.5 KB) - added by William Stein 15 years ago.
sage-1805-rebase_part3.patch (2.0 KB) - added by William Stein 15 years ago.
sage-1805-part4-make_ref_happy.patch (1011 bytes) - added by William Stein 15 years ago.
this should be the final of four patches; it fixes one problem found by the referee (gfurnish)

Download all attachments as: .zip

Change History (20)

comment:1 Changed 15 years ago by William Stein

Owner: changed from failure to William Stein
Status: newassigned

Before:

sage@modular:~/d/sage/sage/structure$ sage -coverage factorization.py 
----------------------------------------------------------------------
factorization.py
SCORE factorization.py: 28% (6 of 21)

Missing documentation:
	 * __init__(self, x, unit=None, cr=False, sort=True)
	 * _set_cr(self, cr)
	 * sort(self)
	 * _cmp(f,g)
	 * _cmp(f,g)
	 * _cmp(f,g)
	 * __reduce__(self)
	 * _cr(self)
	 * _repr_(self)
	 * _latex_(self)
	 * __pow__(self, n)
	 * __invert__(self)
	 * Factorization_deduce_unit(x, mul)


Missing doctests:
	 * unit_part(self)
	 * expand(self)

After:

teragon:structure was$ sage -coverage factorization.py
----------------------------------------------------------------------
factorization.py
SCORE factorization.py: 100% (22 of 22)
----------------------------------------------------------------------

and I fixed numerous conceptual bugs/mistakes in that file.

Changed 15 years ago by William Stein

Attachment: trac-1805.patch added

comment:2 Changed 15 years ago by William Stein

This is a preliminary patch. I'm now doctesting all of sage-2.10 with this patch applied to see what else is broke. I'll post another patch that fixes the problems later.

Changed 15 years ago by William Stein

Attachment: trac-1805-part2.patch added

this part 2 fixes some additional issues, typos, etc., I saw when proofreading my patch.

comment:3 Changed 15 years ago by William Stein

Summary: improve doctest coverage for structure/factorization.py[with patch; needs review] improve doctest coverage for structure/factorization.py

comment:4 Changed 15 years ago by ncalexan

Owner: changed from William Stein to ncalexan
Status: assignednew
Summary: [with patch; needs review] improve doctest coverage for structure/factorization.py[with patch; needs to be merged with 1804] improve doctest coverage for structure/factorization.py

I fixed 1804 not knowing that 1805 did a lot of the same work. Damn! They should both be applied but of course they need to be merged. I'll try to do it tomorrow.

comment:5 Changed 15 years ago by Michael Abshoff

Nick, can you give this another review?

Cheers,

Michael

comment:6 Changed 15 years ago by William Stein

Yes, please review it. My patch fixes a number of subtle serious bugs involving pickling factorizations, which could cause people problems.

comment:7 Changed 15 years ago by Gary Furnish

These patches don't import cleanly against current. They also need the bug doctest removed.

comment:8 Changed 15 years ago by William Stein

Summary: [with patch; needs to be merged with 1804] improve doctest coverage for structure/factorization.py[with patch; needs review] improve doctest coverage for Factorization; fix several critical bugs

I've attached a brand new rebased patch which also fixes a critical bug in factorization (!) which may expose a bug in totallyrealfield, by the way.

I also changed factorizations to be immutable, as suggested by the referee, and they now no longer derive from list, so that cmp works correctly.

Changed 15 years ago by William Stein

new rebased version

Changed 15 years ago by William Stein

Changed 15 years ago by William Stein

comment:9 in reply to:  2 Changed 15 years ago by Gary Furnish

Summary: [with patch; needs review] improve doctest coverage for Factorization; fix several critical bugs[with patch; Positive review pending change] improve doctest coverage for Factorization; fix several critical bugs

sage -t devel/sage-patch1805/build/sage/structure/factorization.py File "factorization.py", line 602:

sage: F = factor(-2*x2 - 1); F

Expected:

(-2.0) * (1.0*x2 + 0.5) * (1.0*x2 + 1.11022302463e-16*x + 0.5)

Got:

(-2.0) * (1.0*x2 - 2.22044604925e-16*x + 0.5) * (1.0*x2 + 0.5)

Positive review pending fix.

comment:10 Changed 15 years ago by Gary Furnish

{{{sage -t devel/sage-patch1805/build/sage/structure/factorization.py File "factorization.py", line 602:

sage: F = factor(-2*x2 - 1); F

Expected:

(-2.0) * (1.0*x2 + 0.5) * (1.0*x2 + 1.11022302463e-16*x + 0.5)

Got:

(-2.0) * (1.0*x2 - 2.22044604925e-16*x + 0.5) * (1.0*x2 + 0.5)

}}}

comment:11 Changed 15 years ago by Gary Furnish

[[[ sage -t devel/sage-patch1805/build/sage/structure/factorization.py File "factorization.py", line 602:

sage: F = factor(-2*x2 - 1); F

Expected:

(-2.0) * (1.0*x2 + 0.5) * (1.0*x2 + 1.11022302463e-16*x + 0.5)

Got:

(-2.0) * (1.0*x2 - 2.22044604925e-16*x + 0.5) * (1.0*x2 + 0.5)

]]]

comment:12 Changed 15 years ago by Gary Furnish

sage -t  devel/sage-patch1805/build/sage/structure/factorization.py**********************************************************************
File "factorization.py", line 602:
    sage: F = factor(-2*x^2 - 1); F
Expected:
    (-2.0) * (1.0*x^2 + 0.5) * (1.0*x^2 + 1.11022302463e-16*x + 0.5)
Got:
    (-2.0) * (1.0*x^2 - 2.22044604925e-16*x + 0.5) * (1.0*x^2 + 0.5)
**********************************************************************

Changed 15 years ago by William Stein

this should be the final of four patches; it fixes one problem found by the referee (gfurnish)

comment:13 Changed 15 years ago by William Stein

Summary: [with patch; Positive review pending change] improve doctest coverage for Factorization; fix several critical bugs[with patch; Positive review] improve doctest coverage for Factorization; fix several critical bugs

I attached the one small change requested.

comment:14 Changed 15 years ago by Michael Abshoff

Resolution: fixed
Status: newclosed

Merged in Sage 2.10.3.rc1

Note: See TracTickets for help on using tickets.