Opened 9 years ago

Closed 2 years ago

# Introduce factor_or_zero into global namespace

Reported by: Owned by: zabrocki minor sage-duplicate/invalid/wontfix factorization vbraun, tscrim, mguaypaq Mike Zabrocki N/A public/ticket/15364/zabrocki/factor_or_zero 1e0e3da44e609c65c2b6b269f80991d01a33b844

### Description

The current top level factor command raises an error if factor is applied to 0. This is inconvenient for trying to factor polynomial expressions which reduce to 0 or lists of integers.

A function which does not force the user to handle 0 as a corner case is SUPER useful for users who just want to observe patterns in factorizations of lists or expressions that might reduce to 0. The error message of factor should indicate to the user to call `factor_or_zero` an error message such as `ArithmeticError: Prime factorization of 0 not defined. Use factor_or_zero to return 0 as the factorization of 0.`

### comment:1 Changed 9 years ago by nbruin

I guess your usage scenario is

```[factor_or_zero(a) for a in some_list_of_expressions]
```

Can't you do:

```[(factor(a) if a !=0 else None) for a in some_list_of_expressions]
```

it's not that much longer (you can save some parentheses if you want) and that way sage doesn't have to choose an arbitrary sentinel value. Plus, the pattern can be used in other similar situations as well.

At some point it's more beneficial to teach users how to use the building blocks available than to provide them with increasingly special-purpose bricks.

### comment:2 Changed 9 years ago by zabrocki

I think that is one of many scenarios. The purpose of `factor` is biased towards number theory. Most of my colleagues use it in algebra to put coefficients in a readable form. I just want to give people who use algebra a less onerous alternative. I don't want to have to put an if statement every time that I factor an algebraic expression.

Solution during Sage Days on Monday (which completely stopped everything for much longer than was necessary):

```sage: map(factor, list_of_coefficients)
ArithmeticError
sage: list_coefficients.remove(0)
sage: map(factor, list_of_coefficients)
```

A more common scenario is

```sage: factor(1/(1-x)-x/(1-x)-1)
ValueError: factorization of 0 not defined
```

Or that a similar expression should come up as the coefficient of some element in an algebra and one should want to `elt.map_coefficients(factor)`.

Last edited 9 years ago by zabrocki (previous) (diff)

### comment:3 Changed 9 years ago by zabrocki

• Type changed from PLEASE CHANGE to enhancement

### comment:4 Changed 9 years ago by zabrocki

• Branch set to public/ticket/15364-factor_or_zero
• Commit set to 9b38cf339cc53f2f6e3e03e18aef94e7a8bda075
• Priority changed from major to minor

New commits:

 ​9b38cf3 Merge remote-tracking branch 'origin/master' into trac_15364 ​c5e89e6 edited more error message about factorization of 0 ​d952c1f added additional error message to Prime factorization of 0. ​de95445 added function factor_or_zero

### comment:5 Changed 9 years ago by git

• Commit changed from 9b38cf339cc53f2f6e3e03e18aef94e7a8bda075 to 16e72e7244e735bcbfc3cc4bf8effb2ea7fae330

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

 ​16e72e7 changes to docstring in factor_or_zero

### comment:6 Changed 9 years ago by zabrocki

• Status changed from new to needs_review

### comment:7 Changed 9 years ago by darij

Please be aware that `Factorization` objects (which is what `factor` normally returns) have some methods which should probably be imitated by the `0` object, or else the can is merely being kicked down the road. For instance, the `value` method of a factorization multiplies it back together, and it is way more useful than it looks from that description:

```sage: Q = FractionField(P)
sage: Q(14*x) / Q(28*(x+1))
14*x/(28*x + 28)
sage: factor(_).value()
1/2*x/(x + 1)
```

0, of course, doesn't have that attribute:

```sage: 0.value()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-4-65708360c0e8> in <module>()
----> 1 Integer(0).value()

/home/darij/gitsage/sage-5.13.beta1/local/lib/python2.7/site-packages/sage/structure/element.so in sage.structure.element.Element.__getattr__ (sage/structure/element.c:3873)()

/home/darij/gitsage/sage-5.13.beta1/local/lib/python2.7/site-packages/sage/structure/misc.so in sage.structure.misc.getattr_from_other_class (sage/structure/misc.c:1696)()

AttributeError: 'sage.rings.integer.Integer' object has no attribute 'value'
```
Last edited 9 years ago by darij (previous) (diff)

### comment:8 Changed 9 years ago by jdemeyer

• Milestone changed from sage-5.13 to sage-6.0

### comment:9 follow-up: ↓ 10 Changed 9 years ago by zabrocki

• Branch changed from public/ticket/15364-factor_or_zero to public/ticket/15364/zabrocki/factor_or_zero
• Commit changed from 16e72e7244e735bcbfc3cc4bf8effb2ea7fae330 to 1e0e3da44e609c65c2b6b269f80991d01a33b844

You are right. For consistency it should return a factorization object rather than an actual 0.

New commits:

 ​1e0e3da should return a factorization object

### comment:10 in reply to: ↑ 9 Changed 9 years ago by nbruin

You are right. For consistency it should return a factorization object rather than an actual 0.

And now compare:

```sage: var('x')
x
sage: factor(x^2-1) #this is just a symbolic_expression
(x - 1)*(x + 1)
sage: PQ.<X>=QQ[]
sage: factor(X^2-1)
(X - 1) * (X + 1)
sage: factor(X^2-1).universe()
Univariate Polynomial Ring in X over Rational Field
sage: PZ.<Y>=ZZ[]
sage: factor(Y^2-1)
(Y - 1) * (Y + 1)
sage: factor(Y^2-1).universe()
Univariate Polynomial Ring in Y over Integer Ring
```

you should probably be consistent with that too.

From the application you describe, I guess you're mostly interested in application to SR (the first case) in which case the most appropriate thing to do is probably to add a method `simplify_factor` to SymbolicExpression?, which could happily simplify the expression "0" to the product "0". In the other cases, the code is just a lot clearer if people specify their own little one-off function that does with 0 what they want.

### comment:11 follow-up: ↓ 13 Changed 9 years ago by nbruin

For Factorization objects: I think you really don't want those to get out into the wild when constructed with 0:

```sage: a=Factorization([(0,1)])
sage: b=Factorization([(2,1)])
sage: a.gcd(b)
1
sage: a^(-1)
0^-1
```

That's just wrong. I think that's even worse than getting an attribute error on `.value()`.

### comment:12 Changed 9 years ago by mguaypaq

• Cc mguaypaq added; ​mguaypaq removed

### comment:13 in reply to: ↑ 11 Changed 9 years ago by zabrocki

• Status changed from needs_review to needs_work

From the application you describe, I guess you're mostly interested in application to SR (the first case) in which case the most appropriate thing to do is probably to add a method `simplify_factor` to SymbolicExpression?, which could happily simplify the expression "0" to the product "0". In the other cases, the code is just a lot clearer if people specify their own little one-off function that does with 0 what they want.

I (and my colleagues that are likely to use this code) will probably be working with algebras over polynomial rings/fraction fields or integer coefficients. SR is rather slow so I don't use it much. This is a convenience function. I can go through my list worksheets and could say that in about 1/3 of them I've had to work around factor throwing errors at me.

For Factorization objects: I think you really don't want those to get out into the wild when constructed with 0:

```sage: a=Factorization([(0,1)])
sage: b=Factorization([(2,1)])
sage: a.gcd(b)
1
sage: a^(-1)
0^-1
```

That's just wrong. I think that's even worse than getting an attribute error on `.value()`.

You are right. We might need to clean up the factorization code so that it handles the 0 object properly.

### comment:14 Changed 8 years ago by vbraun_spam

• Milestone changed from sage-6.0 to sage-6.1

### comment:15 Changed 8 years ago by vbraun_spam

• Milestone changed from sage-6.1 to sage-6.2

### comment:16 Changed 8 years ago by zabrocki

I noticed another problem with `factor` and the factorization objects that it returns. I took a symmetric function expression and used `.map_coefficients(factor)` and my coefficients which were 1 and -1 disappeared (I was applying it to an expression with polynomial coefficients, but you get the idea from the following expression).

```sage: s = SymmetricFunctions(QQ).s()
sage: (s[3,2]+2*s[2,2,1]).map_coefficients(factor)
2*s[2, 2, 1]
```

This happens because `map_coefficients` is calling `s.sum_of_terms` which calls `s.term` which calls `s._from_dict` and we have the strange behavior that `s._from_dict({Partition([3,2]) : factor(1)})` is 0.

If you trace it further, you see that `_from_dict` removes the zeros and computes

`dict( (key, coeff) for key, coeff in d.iteritems() if coeff)`

Now `bool(factor(2))` is `True` while `bool(factor(1))` and `bool(factor(-1))` is `False`. I don't know if there is a good reason that `factor(1)` and `factor(-1)` should be `False` or if this bug should be fixed in `combinat/free_module.py` in the code for `_from_dict`.

### comment:17 Changed 8 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

### comment:18 Changed 8 years ago by vbraun_spam

• Milestone changed from sage-6.3 to sage-6.4

### comment:19 Changed 2 years ago by zabrocki

• Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
• Status changed from needs_work to needs_review

I was the one pushing for a means of gussying up algebraic expressions through a `factor_or_zero` command and this ticket hasn't been modified in 6 years. I propose that it be closed. While I do think that this is a problem that sometimes I am able to find no combination of `expand`, `simplify` and `factor` can clean up a coefficients of an element in an algebra, after the discussions here I don't think that doing that through playing with `factor` is the right way to go.

### comment:20 Changed 2 years ago by klee

• Status changed from needs_review to positive_review

### comment:21 Changed 2 years ago by chapoton

• Resolution set to wontfix
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.