JuliaPy / SymPy.jl

Julia interface to SymPy via PyCall
http://juliapy.github.io/SymPy.jl/
MIT License
268 stars 62 forks source link

Incorrect precedence of `//` in exponent when converting SymPy expression to Julia `Expr` #474

Open rben01 opened 2 years ago

rben01 commented 2 years ago

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 commented 2 years ago

Thanks for the report! A fix is coming soon.

rben01 commented 2 years ago

@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 commented 2 years ago

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!