Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce factor_or_zero into global namespace #15364

Closed
zabrocki mannequin opened this issue Nov 6, 2013 · 16 comments
Closed

Introduce factor_or_zero into global namespace #15364

zabrocki mannequin opened this issue Nov 6, 2013 · 16 comments

Comments

@zabrocki
Copy link
Mannequin

zabrocki mannequin commented Nov 6, 2013

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.

CC: @vbraun @tscrim @mguaypaq

Component: factorization

Author: Mike Zabrocki

Branch/Commit: public/ticket/15364/zabrocki/factor_or_zero @ 1e0e3da

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

@zabrocki zabrocki mannequin added this to the sage-5.13 milestone Nov 6, 2013
@zabrocki zabrocki mannequin added c: factorization labels Nov 6, 2013
@nbruin
Copy link
Contributor

nbruin commented Nov 6, 2013

comment:1

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.

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 6, 2013

comment:2

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

@zabrocki zabrocki mannequin added the t: enhancement label Nov 6, 2013
@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 7, 2013

New commits:

[9b38cf3](https://github.com/sagemath/sagetrac-mirror/commit/9b38cf3)Merge remote-tracking branch 'origin/master' into trac_15364
[c5e89e6](https://github.com/sagemath/sagetrac-mirror/commit/c5e89e6)edited more error message about factorization of 0
[d952c1f](https://github.com/sagemath/sagetrac-mirror/commit/d952c1f)added additional error message to Prime factorization of 0.
[de95445](https://github.com/sagemath/sagetrac-mirror/commit/de95445)added function factor_or_zero

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 7, 2013

Branch: public/ticket/15364-factor_or_zero

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 7, 2013

Commit: 9b38cf3

@zabrocki zabrocki mannequin added p: minor / 4 and removed p: major / 3 labels Nov 7, 2013
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2013

Changed commit from 9b38cf3 to 16e72e7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2013

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

[16e72e7](https://github.com/sagemath/sagetrac-mirror/commit/16e72e7)changes to docstring in factor_or_zero

@zabrocki zabrocki mannequin added the s: needs review label Nov 8, 2013
@darijgr
Copy link
Contributor

darijgr commented Nov 8, 2013

comment:7

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'

@jdemeyer jdemeyer modified the milestones: sage-5.13, sage-6.0 Nov 8, 2013
@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 8, 2013

comment:9

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


New commits:

[1e0e3da](https://github.com/sagemath/sagetrac-mirror/commit/1e0e3da)should return a factorization object

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 8, 2013

Changed commit from 16e72e7 to 1e0e3da

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 8, 2013

@nbruin
Copy link
Contributor

nbruin commented Nov 8, 2013

comment:10

Replying to @zabrocki:

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.

@nbruin
Copy link
Contributor

nbruin commented Nov 8, 2013

comment:11

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().

@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Nov 8, 2013

comment:13

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.

@zabrocki zabrocki mannequin added s: needs work and removed s: needs review labels Nov 8, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.0, sage-6.1 Dec 17, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Mar 12, 2014

comment:16

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.

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@zabrocki
Copy link
Mannequin Author

zabrocki mannequin commented Jan 26, 2020

comment:19

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.

@zabrocki zabrocki mannequin removed this from the sage-6.4 milestone Jan 26, 2020
@zabrocki zabrocki mannequin added s: needs review and removed s: needs work labels Jan 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants