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 precedence of // in exponent when converting SymPy expression to Julia Expr #474

Open
rben01 opened this issue Aug 8, 2022 · 3 comments

Comments

@rben01
Copy link

rben01 commented Aug 8, 2022

When converting a SymPy symbolic expression to a Julia Expr, if there is an integer-division in an exponent, the integer division is placed into the output Expr without parentheses to indicate precedence, which is incorrect.

To reproduce:

julia> convert(Expr, 1/(2*x*sympy.pi^(1//4)))
:((1 / 2) * pi ^ -1//4 * x ^ -1)

Expected result:

:((1 / 2) * pi ^ (-1//4) * x ^ -1)

As is, the returned Expr is interpreted as :(((1 / 2) * pi ^ -1)//4 * x ^ -1), which is incorrect.

@jverzani
Copy link
Collaborator

jverzani commented Aug 8, 2022

Thanks for the report! A fix is coming soon.

@rben01
Copy link
Author

rben01 commented Aug 8, 2022

@jverzani Unfortunately I think the issue is more complicated than my example shows. 1//4 is evaluated as a Rational before being passed to the exponentiation, and although it's displayed as pi ^ -1//4 it's really pi ^ r where r is Rational(1, 4). So my example is not actually problematic.

However I did discover this issue when attempting to convert an expression to Expr, when 1//4 was passed in as integer division instead of as a Rational, and so I got the error (if my memory serves me) ERROR: MethodError: no method matching //(::Int64, ::Float64) (why 4 was being treated as a float, I don't now). I'll try to investigate further.

jverzani added a commit to jverzani/SymPy.jl that referenced this issue Aug 8, 2022
jverzani added a commit that referenced this issue Aug 8, 2022
@jverzani
Copy link
Collaborator

jverzani commented Aug 8, 2022

The fix I just merged basically throws in parentheses to evaluate ()^(). Before, without that the precedence rules were not consistent with the expression. Before tagging, can you send along your example that fails and I'll make sure this addresses that. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants