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

Changed Printing of Logic Expressions #11448

Closed
wants to merge 2 commits into from

Conversation

arihantparsoya
Copy link
Contributor

Fix for #11435.

  • Changed the functions _print_And() _print_Or() to print & and |
    symbols in the expressions instead of And and Or.

Sample Program

>>> from sympy import *
>>> from sympy.abc import x,y,z
>>> And(Not(x), Or(y, z))
((y | z) & ~(x))
  • Previous tests are modified to account for new changes.

Fix for sympy#11435.

Changed the functions _print_And() _print_Or() to print `&` and `|`
symbols in the expressions instead of `And` and `Or`.

Previously, printing module used its superclass to print `Not`
expression. Hence, a new function _print_Not() is created to change the
printing of `Not`expressions.

Previous tests are modified to account for new changes.
@arihantparsoya
Copy link
Contributor Author

@asmeurer , can your review this?

Currently, the tests are failing due to the use of old And , Or and Not. I will change them if the current changes made are correct.

@arihantparsoya
Copy link
Contributor Author

@asmeurer , ping.


A = Exponential('a', 1)
B = Exponential('b', 1)
assert str(pspace(Tuple(A, B)).domain) == "Domain: And(0 <= a, 0 <= b, a < oo, b < oo)"
assert str(pspace(Tuple(A, B)).domain) == "Domain: (0 <= a & 0 <= b & a < oo & b < oo)"
Copy link
Member

Choose a reason for hiding this comment

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

You need to be more careful about parenthesization. This is wrong (needs parentheses around the inequalities).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this output desirable?

>>> from sympy import *
>>> from sympy.abc import x,y,z
>>> print(And(Not(x), Or(y, z)))
((((y) | (z))) & (~(x)))

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, I don't think that is desirable.
Is it possible to parenthesize only when necessary, depending on operator precedence?

I would suggest you take a look at the parethesize method of the StrPrinter class.
(https://github.com/sympy/sympy/blob/master/sympy/printing/str.py#L27)

Personally, I think this would be desirable:

>>> print(And(Not(x), Or(y, z)))
(y | z) & ~x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am able to get the output using the parethesize method method. But I am unable to parenthesize inequalities as desired. Any tips?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you used the precedence function, which follows this precedence table:
https://github.com/sympy/sympy/blob/master/sympy/printing/precedence.py#L8

Do we really need parentheses around the inequalities in that case? Don't relational operators have precedence over logical And? (at least from a programming language perspective)

However, this does look more appealing:

"Domain: (0 <= a) & (0 <= b) & (a < oo) & (b < oo)"

Copy link
Member

Choose a reason for hiding this comment

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

Do we really need parentheses around the inequalities in that case? Don't relational operators have precedence over logical And? (at least from a programming language perspective)

No they don't. That's why parentheses are so important.

In [71]: type(x < y & z)
Out[71]: sympy.core.relational.StrictLessThan

In [72]: x < (y & z)
Out[72]: x < y ∧ z

In [73]: (x < y) & z
Out[73]: z ∧ x < y

In [74]: (x < y & z).args
Out[74]: (x, y ∧ z)

See also https://docs.python.org/3/reference/expressions.html#operator-precedence

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, I find the behavior in the example above odd. I would expect the following results to be equal:

>>> srepr(x < y & z)
"StrictLessThan(Symbol(′x′),And(Symbol(′y′),Symbol(′z′)))"

>>> srepr((x < y) & z)
"And(Symbol(′z′),StrictLessThan(Symbol(′x′),Symbol(′y′)))"

As for the python reference, it agrees with what I was trying to say in my previous answer.

>>> 2 > 2 and True
False

>>> (2 > 2) and True
False

>>> 2 > (2 and True)
True

Copy link
Member

Choose a reason for hiding this comment

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

@gorisaka do not confuse and and &. The former has lower precedence than the inequality operators, and can't be overridden by libraries like SymPy. & has higher precedence and can be overridden.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Since | is also overridden the same problem occurs with it:

>>> str((x > 1) | (x < -1))
x > 1 | x < -1

used function parenthesize to manage parentheses in logical expressions.

def _print_Not(self, expr):
return '%s(%s)' % ('~', self._print(expr.args[0]))
return '%s%s' % ('~', self._print(expr.args[0]))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect:

>>> print(Not(Or(x, y)))
~x | y

Not also needs parenthesization depending on precedence.

@czgdp1807
Copy link
Member

The associated issue has been fixed,

>>> print(And(Not(a), Or(b, c)))
~a & (b | c)

Closing this PR. Thanks for your contributions.

@czgdp1807 czgdp1807 closed this Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants