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
Conversation
✅ Hi, I am the SymPy bot (v162). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.
Click here to see the pull request description that was parsed.
|
Previously both `PrettyPrinter._print_Mul` and `prettyForm.__mul__` tried to apply parenthesis, and the action of the former hid a typo in the latter.
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) Full benchmark results can be found as artifacts in GitHub Actions |
@@ -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" |
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
)
Anyone object to merging this? Again, it may not be the end of issues related to unevaluated multiplications, but it is a step in the right direction and will also make later changes easier as the code quality improves. I can fix the merge conflict if you prefer @eric-wieser , but I'll give you a few days to sort it out if you prefer and anyone to object against merging. |
Previously both
PrettyPrinter._print_Mul
andprettyForm.__mul__
tried to apply parenthesis, and the action of the former hid a typo in the latter.References to other Issues or PRs
Fixes #21814
#21844 (comment) draws attention to some weird code.
Brief description of what is fixed or changed
The
arg
variable was assigned but not used. The intention was probably to useargs
in its place, but the code reads better withargs
below replaced witharg
instead.This bug had no effect, because the broken code was duplicated without the bug somewhere else.
Other comments
Release Notes
NO ENTRY