Symbolic factor_list() should do integer factorisation of integers/rationals

[rwst]

Let symbolic factor_list() do integer factorisation if given an integer or fraction.

sage: SR(50).factor_list()
[(50, 1)]
sage: SR(49/100).factor_list()
[(49/100, 1)]

Too much surprise for new users.

Component: symbolics

Author: Ralf Stephan, Vincent Delecroix

Branch/Commit: u/rws/21067 @ a821fb6

Reviewer: Daniel Krenn

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

[rwst]

Branch: u/rws/symbolic_factor___should_do_integer_factorisation_of_integers_rationals

[rwst]

Author: Ralf Stephan

[rwst]

New commits:

ac3d7cb

21067: do ZZ/QQ factorization in ex.factor_list()

[rwst]

Commit: ac3d7cb

[tscrim]

comment:4

One minor thing: could you move the import into the factor_list method? I worry that this could lead to import hell.

[nbruin]

comment:5

Two comments:

• I don't think SR needs to be imported at all. The very fact that we land in this factor_list implementation should mean that self.parent() should be SR. And if it isn't, that's probably the better parent to use anyway.

• If we're going to do this anyway, I think it's better if we have

sage: (5/3*x/(x+1)).factor_list()
[(x + 1, -1), (x, 1), (5, 1), (3,-1)]

because it's closer to the idea that the factorisation of a product U*V has a tendency to be the concatenation of the factorisations of U and V.

It should be clear that factor_list just returns a factorisation:

sage: ((sqrt(5)+1)^2*(x^2-5)).expand_rational().factor_list()
[(x^2 - 5, 1), (sqrt(5) + 3, 1), (2, 1)]

but keeping the convention that we factor rational numbers into their prime factors should be fine and could be nice for newbies.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

eca869e

21067: extend functionality

[sagetrac-git]

Changed commit from ac3d7cb to eca869e

[videlec]

comment:7

Instead of having a special check and a special method for integer/rationals couldn't we use something simpler and more generic like

def _factorization_from_pyobject(self):
    try:
        a = self.pyobject()
    except TypeError:
        return

    try:
        f = a.factor()
    except (AttributeError, NotImplementedError):
        return

    # from here I assume that a is a Sage object which might not be the case...
    P = self.parent()
    facs = [(P(p), P(e)) for p,e in f]
    if f.unit().is_one():
        return facs
    else:
        return [(P(f.unit()), P(1))] + facs

[rwst]

comment:8

Vincent, if you think it's better, then please go ahead on a new branch. I'm not so deep in Python that I could object.

[nbruin]

comment:9

I think "factor" in general could be a little fickle to depend on:

sage: K.<a>=NumberField(x^2+5)
sage: K(2).factor()
ArithmeticError: non-principal ideal in factorization
sage:  SR(2).pyobject().factor()
2
sage: (2+I-I).pyobject().factor() 
(-I) * (I + 1)^2

[rwst]

comment:11

I think this ticket should also do:

sage: (2+6*x).factor_list()
[(3*x + 1, 1), (2, 1)]

[rwst]

Changed branch from u/rws/symbolic_factor___should_do_integer_factorisation_of_integers_rationals to u/rws/21067

[rwst]

Changed author from Ralf Stephan to Ralf Stephan, Vincent Delecroix

[rwst]

Changed commit from eca869e to 4769429

[rwst]

comment:13

I used Vincent's code for a new version that tries also to separate the content in a univariate polynomial.

---

New commits:

4769429

21067: Symbolic factor_list() should do integer factorisation of

[sagetrac-git]

Changed commit from 4769429 to 8091ff3

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

8091ff3

Merge branch 'develop' into t/21067/21067

[rwst]

comment:15

Doctest errors, see patchbot.

[sagetrac-git]

Changed commit from 8091ff3 to a821fb6

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

933a75f	Merge branch 'develop' into t/21067/21067
a821fb6	21067: simplification, doctest fix

[dkrenn]

comment:18

LGTM, I only have one comment:

+            raise ValueError

This is only used internally, so it might be good to consider a customized exception:

class CannotFactor(RuntimeError):
    pass

And yes, I think RuntimeError is more appropriate (but if you insist on ValueError then I am good with it as well, although wanting to hear your arguments for that ;) )

[dkrenn]

Reviewer: Daniel Krenn