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

Incorrect printing of non-evaluated multiplication with negative number #16955

Open
jeanas opened this issue Jun 3, 2019 · 5 comments
Open

Comments

@jeanas
Copy link

jeanas commented Jun 3, 2019

This expression:

Mul(a, b, evaluate=False)

will be incorrectly represented by repr(), srepr() and latex() when a is a negative number (either integer or float).

Examples:

>>> expr = Mul(-2, 5, evaluate=False)
>>> 
>>> repr(expr) # expecting '-2*5'
'-10'
>>> srepr(expr) # expecting 'Mul(Integer(-2), Integer(5))'
'Mul(Integer(-1), Integer(2), Integer(5))'
>>> # this is definitely wrong because:
... expr.args
(-2, 5)
>>> 
>>> latex(expr)
'- 10'
>>> 
>>> # but... on my plain console (ie no IPython):
... init_printing()
>>> expr
-25

Tested under SymPy 1.4 with Python 3.8.0a2.

@oscargus
Copy link
Contributor

oscargus commented Jun 8, 2019

Some more observations:

In [1]: repr(Mul(2, -5, evaluate=False))
Out[1]: '(-5)*2'
In [2]: srepr(Mul(2, -5, evaluate=False))
Out[2]: 'Mul(Integer(-5), Integer(2))'
In [3]: latex(Mul(2, -5, evaluate=False))
Out[3]: '\\left(-5\\right) 2'

so as you say, it is when the first argument is negative that it somehow messes up.

@oscargus
Copy link
Contributor

oscargus commented Jun 8, 2019

And some more:

In [37]: latex(Mul(-2, -5, evaluate=False))
Out[37]: '- -10'
In [38]: repr(Mul(-2, -5, evaluate=False))
Out[38]: '-(-10)'
In [39]: srepr(Mul(-2, -5, evaluate=False))
Out[39]: 'Mul(Integer(-5), Integer(-1), Integer(2))'

@oscargus
Copy link
Contributor

oscargus commented Jun 8, 2019

Parts of it come from this function:

sympy/sympy/core/mul.py

Lines 1746 to 1802 in 08644cb

def _keep_coeff(coeff, factors, clear=True, sign=False):
"""Return ``coeff*factors`` unevaluated if necessary.
If ``clear`` is False, do not keep the coefficient as a factor
if it can be distributed on a single factor such that one or
more terms will still have integer coefficients.
If ``sign`` is True, allow a coefficient of -1 to remain factored out.
Examples
========
>>> from sympy.core.mul import _keep_coeff
>>> from sympy.abc import x, y
>>> from sympy import S
>>> _keep_coeff(S.Half, x + 2)
(x + 2)/2
>>> _keep_coeff(S.Half, x + 2, clear=False)
x/2 + 1
>>> _keep_coeff(S.Half, (x + 2)*y, clear=False)
y*(x + 2)/2
>>> _keep_coeff(S(-1), x + y)
-x - y
>>> _keep_coeff(S(-1), x + y, sign=True)
-(x + y)
"""
if not coeff.is_Number:
if factors.is_Number:
factors, coeff = coeff, factors
else:
return coeff*factors
if coeff is S.One:
return factors
elif coeff is S.NegativeOne and not sign:
return -factors
elif factors.is_Add:
if not clear and coeff.is_Rational and coeff.q != 1:
q = S(coeff.q)
for i in factors.args:
c, t = i.as_coeff_Mul()
r = c/q
if r == int(r):
return coeff*factors
return Mul(coeff, factors, evaluate=False)
elif factors.is_Mul:
margs = list(factors.args)
if margs[0].is_Number:
margs[0] *= coeff
if margs[0] == 1:
margs.pop(0)
else:
margs.insert(0, coeff)
return Mul._from_args(margs)
else:
return coeff*factors

especially the last line that just returns the product unless something else has triggered. The product is for coeff = 2 and factors = 5, as the following piece of code has removed the sign:

sympy/sympy/printing/str.py

Lines 270 to 275 in 08644cb

c, e = expr.as_coeff_Mul()
if c < 0:
expr = _keep_coeff(-c, e)
sign = "-"
else:
sign = ""

This is for str/repr, but most likely the other printers call this code in a similar way. The reason, I assume, is to try to be a bit clever when printing multiplications, see the examples of _keep_coeff. Clearly, this type of use was not considered.

@AaronStiff
Copy link
Contributor

I know this issue is over a year old, but I just wanted to update it. With Sympy 1.7.1 (and 1.8.dev), repr and latex are no longer returning wrong results for these unevaluated expressions. However, it appears more complex unevaluated expressions are still causing problems (see #19889 as mentioned).

Also, srepr is still returning the results described above.

@AaronStiff
Copy link
Contributor

First, a workaround: for every srepr case described above, passing order = 'old' to srepr preserves the order of the factors and prevents -1 from being added as a factor:

>>> from sympy import *
>>> srepr(Mul(-2, 5, evaluate=False), order = 'old')
'Mul(Integer(-2), Integer(5))'
>>> srepr(Mul(2, -5, evaluate=False), order = 'old')
'Mul(Integer(2), Integer(-5))'
>>> srepr(Mul(-2, -5, evaluate=False), order = 'old')
'Mul(Integer(-2), Integer(-5))'

However, the reason this works is because it bypasses Mul.as_ordered_factors(), and thus Expr.args_cnc(), which purposefully separates -1 from the rest of the factors:

sympy/sympy/core/expr.py

Lines 1297 to 1310 in 23d11f3

def args_cnc(self, cset=False, warn=True, split_1=True):
"""Return [commutative factors, non-commutative factors] of self.
Explanation
===========
self is treated as a Mul and the ordering of the factors is maintained.
If ``cset`` is True the commutative factors will be returned in a set.
If there were repeated factors (as may happen with an unevaluated Mul)
then an error will be raised unless it is explicitly suppressed by
setting ``warn`` to False.
Note: -1 is always separated from a Number unless split_1 is False.

Not sure if this behavior is preferred, or if it should be changed to reflect what people will expect.

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

4 participants