Opened 12 years ago

Closed 12 years ago

#1805 closed defect (fixed)

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

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

Description


Attachments (6)

trac-1805.patch (22.3 KB) - added by was 12 years ago.
trac-1805-part2.patch (5.7 KB) - added by was 12 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 was 12 years ago.
new rebased version
sage-1805-part2-rebased.patch (3.5 KB) - added by was 12 years ago.
sage-1805-rebase_part3.patch (2.0 KB) - added by was 12 years ago.
sage-1805-part4-make_ref_happy.patch (1011 bytes) - added by was 12 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 12 years ago by was

  • Owner changed from failure to was
  • Status changed from new to assigned

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 12 years ago by was

comment:2 follow-up: Changed 12 years ago by was

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 12 years ago by was

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

comment:3 Changed 12 years ago by was

  • Summary changed from improve doctest coverage for structure/factorization.py to [with patch; needs review] improve doctest coverage for structure/factorization.py

comment:4 Changed 12 years ago by ncalexan

  • Owner changed from was to ncalexan
  • Status changed from assigned to new
  • Summary changed from [with patch; needs review] improve doctest coverage for structure/factorization.py to [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 12 years ago by mabshoff

Nick, can you give this another review?

Cheers,

Michael

comment:6 Changed 12 years ago by was

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

comment:7 Changed 12 years ago by gfurnish

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

comment:8 Changed 12 years ago by was

  • Summary changed from [with patch; needs to be merged with 1804] improve doctest coverage for structure/factorization.py to [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 12 years ago by was

new rebased version

Changed 12 years ago by was

Changed 12 years ago by was

comment:9 in reply to: ↑ 2 Changed 12 years ago by gfurnish

  • Summary changed from [with patch; needs review] improve doctest coverage for Factorization; fix several critical bugs to [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 12 years ago by gfurnish

{{{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 12 years ago by gfurnish

[[[ 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 12 years ago by gfurnish

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 12 years ago by was

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

comment:13 Changed 12 years ago by was

  • Summary changed from [with patch; Positive review pending change] improve doctest coverage for Factorization; fix several critical bugs to [with patch; Positive review] improve doctest coverage for Factorization; fix several critical bugs

I attached the one small change requested.

comment:14 Changed 12 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 2.10.3.rc1

Note: See TracTickets for help on using tickets.