-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add code printer for KroneckerDelta #18185
Conversation
✅ Hi, I am the SymPy bot (v150). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like: This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6. Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
Codecov Report
@@ Coverage Diff @@
## master #18185 +/- ##
=============================================
+ Coverage 74.959% 75.347% +0.388%
=============================================
Files 642 640 -2
Lines 167079 167177 +98
Branches 39312 39426 +114
=============================================
+ Hits 125241 125964 +723
+ Misses 36305 35675 -630
- Partials 5533 5538 +5 |
This looks good but it will need to be tested. Can you add a test in |
sympy/printing/pycode.py
Outdated
def _print_KroneckerDelta(self, expr): | ||
a, b = expr.args | ||
|
||
return '1 if ({a}) == ({b}) else 0'.format( |
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 actually meant the entire expression should be in parentheses. For example, pycode(KroneckerDelta(x, y) + 1)
is wrong. For the inner expression it should use self.parenthesize
, although I think there won't actually be any cases where it is needed because ==
has very high precedence in Python.
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.
Ah, I see. I'm not sure I know how to use self.parenthesize
… Should it be something like this?
a, b = expr.args
PREC = precedence(expr)
a = self.parenthesize(a, PREC)
b = self.parenthesize(b, PREC)
return '(1 if {a} == {b} else 0)'.format(...)
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.
Yes, except I'm not sure precedence(expr)
will work without a change to precedence
.
You can check with some operators that have higher precedence than ==
https://docs.python.org/3/reference/expressions.html#operator-precedence. The only relevant one I see that would be relevant are logic operations, which don't actually make sense inside of a KroneckerDelta, but I guess you can put them there to test.
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.
After reading through precedence
and parenthesize
, I think this code would've worked:
a, b = expr.args
PREC = PRECEDENCE['Relational']
a = self.parenthesize(a, PREC)
b = self.parenthesize(b, PREC)
return '(1 if {a} == {b} else 0)'.format(...)
But the actual implementation of KroneckerDelta
relies on the argument being numbers, not logical expressions, and it threw an error for KroneckerDelta(And(...), ...)
. So I'm just leaving the parenthesize
out and going with a simpler version.
Ping! I think this PR is ready to be merged. |
I'm not very familiar with the code printers. How would I test this manually? |
The Python code printer is used in from sympy import *
x, y = symbols('x y')
f = KroneckerDelta(x, y)
delta = lambdify((x, y), f)
delta(1, 2)
# 0
delta(2, 2)
# 1 To see the actual code output, you can use the from sympy.printing.pycode import pycode
pycode(f)
# '(1 if x==y else 0)' |
This looks good. Thanks! |
I'd like to add that this approach broke my setup for generating matrices out of matrix elemtens with KroneckerDeltas in it since it doesn't permit np.array inputs. I found a workaround by replacing the KroneckerDelta before a lambdify with a generic function and then define the np.eye matrix as that generic function. Maybe there is a cleaner way to account for such a use case, I'd gladly learn about it... I used a custom function mapping before, like so:
|
@maclomhair I think it would be good to open a new issue to discuss that. If this has broken your code since the 1.6 release it would be good to show an example of the code that was working before and what has changed. |
References to other Issues or PRs
Maybe related to #17396
Brief description of what is fixed or changed
Added python printer for KroneckerDelta
Other comments
Release Notes