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

printing/pretty: Do not compute parenthesization twice #22236

Merged
merged 5 commits into from Nov 19, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 4 additions & 20 deletions sympy/printing/pretty/pretty.py
Expand Up @@ -223,7 +223,7 @@ def _print_Relational(self, e):

l = self._print(e.lhs)
r = self._print(e.rhs)
pform = prettyForm(*stringPict.next(l, op, r))
pform = prettyForm(*stringPict.next(l, op, r), binding=prettyForm.OPEN)
return pform

def _print_Not(self, e):
Expand Down Expand Up @@ -1873,25 +1873,9 @@ def _print_Mul(self, product):
else:
a.append(item)

from sympy import Integral, Piecewise, Product, Sum

# Convert to pretty forms. Add parens to Add instances if there
# is more than one term in the numer/denom
for i in range(0, len(a)):
if (a[i].is_Add and len(a) > 1) or (i != len(a) - 1 and
isinstance(a[i], (Integral, Piecewise, Product, Sum))):
a[i] = prettyForm(*self._print(a[i]).parens())
elif a[i].is_Relational:
a[i] = prettyForm(*self._print(a[i]).parens())
else:
a[i] = self._print(a[i])

for i in range(0, len(b)):
if (b[i].is_Add and len(b) > 1) or (i != len(b) - 1 and
isinstance(b[i], (Integral, Piecewise, Product, Sum))):
b[i] = prettyForm(*self._print(b[i]).parens())
else:
b[i] = self._print(b[i])
# Convert to pretty forms. Parentheses are added by `__mul__`.
a = [self._print(ai) for ai in a]
b = [self._print(bi) for bi in b]

# Construct a pretty form
if len(b) == 0:
Expand Down
15 changes: 9 additions & 6 deletions sympy/printing/pretty/stringpict.py
Expand Up @@ -439,18 +439,21 @@ def __mul__(self, *others):
}

if len(others) == 0:
return self # We aren't actually multiplying... So nothing to do here.
args = self
if args.binding > prettyForm.MUL:
arg = stringPict(*args.parens())
result = [args]
return self # We aren't actually multiplying... So nothing to do here.

# add parens on args that need them
arg = self
if arg.binding > prettyForm.MUL and arg.binding != prettyForm.NEG:
arg = stringPict(*arg.parens())
result = [arg]
for arg in others:
if arg.picture[0] not in quantity.values():
result.append(xsym('*'))
#add parentheses for weak binders
if arg.binding > prettyForm.MUL:
if arg.binding > prettyForm.MUL and arg.binding != prettyForm.NEG:
arg = stringPict(*arg.parens())
result.append(arg)

len_res = len(result)
for i in range(len_res):
if i < len_res - 1 and result[i] == '-1' and result[i + 1] == xsym('*'):
Expand Down
30 changes: 28 additions & 2 deletions sympy/printing/pretty/tests/test_pretty.py
Expand Up @@ -950,6 +950,8 @@ def test_negative_fractions():
"""
assert pretty(expr) == ascii_str
assert upretty(expr) == ucode_str

def test_Mul():
expr = Mul(0, 1, evaluate=False)
assert pretty(expr) == "0*1"
assert upretty(expr) == "0⋅1"
Expand All @@ -975,8 +977,8 @@ def test_negative_fractions():
assert pretty(expr) == "0 + 0 + 1"
assert upretty(expr) == "0 + 0 + 1"
expr = Mul(1, -1, evaluate=False)
assert pretty(expr) == "1*(-1)"
assert upretty(expr) == "1⋅(-1)"
assert pretty(expr) == "1*-1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's a good idea to have two operators next to each other even though semantically the result is the same, hence, also, x**(-2).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean "semantically the result is the same"? The same as what? (1*-)1 is nonsense, so there's only one possible interpretation of how to parse this in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct that there is only one possible interpretation, but I am quite sure there will be new issues opened about not have brackets around -1...

What happens with, e.g., Add(0, -1, evaluate=False)?

(In general, I think that it is a great PR, removing unnecessary code.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't touch the code for Add; but I assume your point is that if Add uses parens here, mul should too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By semantics I mean only that the parentheses are redundant but to see two operators next to each other -- though legal, semantically -- is odd (e.g. the valid 1--2 == 3)

assert upretty(expr) == "1⋅-1"
expr = Mul(1.0, x, evaluate=False)
assert pretty(expr) == "1.0*x"
assert upretty(expr) == "1.0⋅x"
Expand All @@ -995,6 +997,30 @@ def test_negative_fractions():
expr = Mul(Rational(2, 3), Rational(5, 7), evaluate=False)
assert pretty(expr) == "2/3*5/7"
assert upretty(expr) == "2/3⋅5/7"
expr = Mul(x + y, Rational(1, 2), evaluate=False)
namannimmo10 marked this conversation as resolved.
Show resolved Hide resolved
assert pretty(expr) == "(x + y)*1/2"
assert upretty(expr) == "(x + y)⋅1/2"
expr = Mul(Rational(1, 2), x + y, evaluate=False)
assert pretty(expr) == "x + y\n-----\n 2 "
assert upretty(expr) == "x + y\n─────\n 2 "
expr = Mul(S.One, x + y, evaluate=False)
assert pretty(expr) == "1*(x + y)"
assert upretty(expr) == "1⋅(x + y)"
expr = Mul(x - y, S.One, evaluate=False)
assert pretty(expr) == "(x - y)*1"
assert upretty(expr) == "(x - y)⋅1"
expr = Mul(Rational(1, 2), x - y, S.One, x + y, evaluate=False)
assert pretty(expr) == "1/2*(x - y)*1*(x + y)"
assert upretty(expr) == "1/2⋅(x - y)⋅1⋅(x + y)"
expr = Mul(x + y, Rational(3, 4), S.One, y - z, evaluate=False)
assert pretty(expr) == "(x + y)*3/4*1*(y - z)"
assert upretty(expr) == "(x + y)⋅3/4⋅1⋅(y - z)"
expr = Mul(x + y, Rational(1, 1), Rational(3, 4), Rational(5, 6),evaluate=False)
assert pretty(expr) == "(x + y)*1*3/4*5/6"
assert upretty(expr) == "(x + y)⋅1⋅3/4⋅5/6"
expr = Mul(Rational(3, 4), x + y, S.One, y - z, evaluate=False)
assert pretty(expr) == "3/4*(x + y)*1*(y - z)"
assert upretty(expr) == "3/4⋅(x + y)⋅1⋅(y - z)"

def test_issue_5524():
assert pretty(-(-x + 5)*(-x - 2*sqrt(2) + 5) - (-y + 5)*(-y + 5)) == \
Expand Down