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

[williamstein]

Component: doctest coverage

Issue created by migration from https://trac.sagemath.org/ticket/1805

[williamstein]

comment:1

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.

[williamstein]

comment:2

Attachment: trac-1805.patch.gz

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.

[williamstein]

Attachment: trac-1805-part2.patch.gz

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

[ncalexan]

comment:4

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.

[sagetrac-mabshoff]

comment:5

Nick, can you give this another review?

Cheers,

Michael

[williamstein]

comment:6

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

[garyfurnish]

comment:7

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

[williamstein]

comment:8

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.

[williamstein]

new rebased version

[williamstein]

Attachment: sage-1805-rebased_2.10.3.patch.gz

Attachment: sage-1805-part2-rebased.patch.gz

[williamstein]

Attachment: sage-1805-rebase_part3.patch.gz

[garyfurnish]

comment:9

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

---

Positive review pending fix.

[garyfurnish]

comment:10

{{{sage -t devel/sage-patch1805/build/sage/structure/factorization.py

---

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

---

}}}

[garyfurnish]

comment:11

[[[
sage -t devel/sage-patch1805/build/sage/structure/factorization.py

---

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

---

]]]

[garyfurnish]

comment:12

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)
**********************************************************************

[williamstein]

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

[williamstein]

comment:13

Attachment: sage-1805-part4-make_ref_happy.patch.gz

I attached the one small change requested.

[sagetrac-mabshoff]

comment:14

Merged in Sage 2.10.3.rc1