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

Adding an extension for SymPy -> Symbolics.jl using PythonCall.jl #957

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jClugstor
Copy link

This is my rough attempt at converting SymPy expressions to Symbolics.jl expressions. It uses PythonCall.jl 's support for custom rules to define conversion rules. Not sure if this should go in the same file as SymbolicsSymPyExt, but I put it in a different one and a different module since it uses PythonCall instead of PyCall.

Essentially when the pyconvert function is called to convert a SymPy Symbol to a Symbolics Num, pyconvert returns a Symbolics variable with the same name as the SymPy Symbol. Then for any other type of expression pyconvert can be called recursively on the arguments to turn them in to expressions usable with Symbolics or ModelingToolkit.

Some issues:

The names of the variables are not exposed when they are converted, so you can't use them... If they were istree I think I could go through to all of the variables and make them in to Julia variables, but they don't seem to come out as istree objects.

Each conversion from Symbol to Num just makes a new variable. So if a variable already existed with the same name as something in another expression you were trying to convert, I think it would just be overwritten. Maybe solved by adding a check to see if a variable of the same name exists already?

Comment on lines 12 to 14
CondaPkg.add("sympy")

sp = pyimport("sympy")
Copy link
Member

Choose a reason for hiding this comment

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

That shouldn't happen in an extension

Comment on lines 96 to 121
# little tests
PythonCall.pyconvert(Symbolics.Num, sp.sympify("t"))

PythonCall.pyconvert(Symbolics.Num, sp.sympify("t**t"))

PythonCall.pyconvert(Symbolics.Num, sp.sympify("t + z + d"))

PythonCall.pyconvert(Symbolics.Num, sp.sympify("t*z*9"))

PythonCall.pyconvert(Symbolics.Num, sp.sympify("5*t*z + 3*d + h/(b*5)"))

PythonCall.pyconvert(Symbolics.Num, sp.sympify("t * n/z * t**4 * h**z + l*h - j"))

PythonCall.pyconvert(Symbolics.Equation, sp.sympify("Eq(2,5, evaluate = False)"))

PythonCall.pyconvert(Symbolics.Equation, sp.sympify("Eq(t*x + 5**x + 20/t, 90/t + t**4 - t*z)"))

PythonCall.pyconvert(Symbolics.Num, sp.sympify("Function('f')(x)"))

PythonCall.pyconvert(Symbolics.Num, sp.sympify("f(x,y)"))

PythonCall.pyconvert(Symbolics.Equation, sp.sympify("Eq(f(x), 2*x +1)"))

PythonCall.pyconvert(Symbolics.Num,sp.sympify("Derivative(f(x),x)"))

PythonCall.pyconvert(Symbolics.Num,sp.sympify("Eq(Derivative(f(x),x), x"))
Copy link
Member

Choose a reason for hiding this comment

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

add to the tests?

@ChrisRackauckas
Copy link
Member

You might want to test this under the assumption that https://github.com/jverzani/SymPyPythonCall.jl may merge?

@jverzani

@jverzani
Copy link
Contributor

You might want to test this under the assumption that https://github.com/jverzani/SymPyPythonCall.jl may merge?

That was recently tagged as a standalone package. I added a TermInterface extension to SymPy and could add that to SymPyPythonCall if it would be helpful here, but I'm not sure how the conversion to Symbolics variables will interact with the conversions already performed in SymPyCall.

@jClugstor
Copy link
Author

As far as I can tell there shouldn't be any kind of conflict, with this you have to explicitly tell pyconvert what Julia type to convert to, which for this is just Symbolics.Num and Symbolics.Equation. As it is I don't think any conversions would interact at all since I'm basically just making new Nums from all of the SymPy symbols in a given expression.

@jverzani
Copy link
Contributor

As far as I can tell there shouldn't be any kind of conflict, with this you have to explicitly tell pyconvert what Julia type to convert to, which for this is just Symbolics.Num and Symbolics.Equation. As it is I don't think any conversions would interact at all since I'm basically just making new Nums from all of the SymPy symbols in a given expression.

I'm trying to mirror your work in SymPyPythonCall, but must be missing something, as I can't seem to trigger the added conversion rules. (jverzani/SymPyPythonCall.jl#17). If you had time, I'd very much appreciate a look over.

@jClugstor
Copy link
Author

How are you trying to trigger the rules?

@jverzani
Copy link
Contributor

How are you trying to trigger the rules?

Here is what I think should work (please correct if I'm missing something):

julia> using Revise; using SymPyPythonCall, Symbolics
[ Info: Precompiling SymPyPythonCallSymbolicsExt [4ef2779d-d505-5162-aad0-0e1f452ed212]

julia> SymPyPythonCall.@syms x
(x,)

julia> u = x.py
Python: u

julia> SymPyPythonCall.PythonCall.pyconvert(Symbolics.Num, u)
ERROR: cannot convert this Python 'Symbol' to a Julia 'Num'
...

If I do things directly, it seems to work as desired

julia> using SymPyPythonCall

julia> const sp = SymPyPythonCall.sympy.py
Python: <module 'sympy' from '/Users/verzani/.julia/environments/v1.9/.CondaPkg/env/lib/python3.11/site-packages/sympy/__init__.py'>

julia> const PythonCall = SymPyPythonCall.PythonCall
PythonCall

julia> import SymPyPythonCall.PythonCall: Py, pyisinstance, pyconvert

julia> function pyconvert_rule_sympy_symbol(::Type{Symbolics.Num}, x::Py)
           if !pyisinstance(x,sp.Symbol)
               return PythonCall.pyconvert_unconverted()
           end
           name = PythonCall.pyconvert(Symbol,x.name)
           return PythonCall.pyconvert_return(Symbolics.variable(name))
       end
pyconvert_rule_sympy_symbol (generic function with 1 method)

julia> pyconvert_rule_sympy_symbol(Symbolics.Num, u)
x

This leads me to guess, the issue comes in this identification:

PythonCall.pyconvert_add_rule("sympy.core.symbol:Symbol", Symbolics.Num, pyconvert_rule_sympy_symbol)

The class of u seems correct though:

julia> u.__class__
Python: <class 'sympy.core.symbol.Symbol'>

@jClugstor
Copy link
Author

The problem could be with the !pyisinstance checks in the rules functions. Maybe it's returning a pyconvert_unconverted() because it doesn't recognize sp.Symbol? I ended up getting rid of those for now, even though technically the functions should return either pyconvert_return or a pyconvert_unconverted.

@jverzani
Copy link
Contributor

Thanks, I'll have look.

@shashi
Copy link
Member

shashi commented Aug 28, 2023

Just a heads up @jverzani we don't use TermInterface.jl anymore; the interface has been moved back into SymbolicUtils for some stability/compatibility reasons, but potentially in the future we can use it.

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2023

Codecov Report

Merging #957 (48968c0) into master (9c3a18a) will increase coverage by 0.33%.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##           master    #957      +/-   ##
=========================================
+ Coverage    7.93%   8.27%   +0.33%     
=========================================
  Files          26      27       +1     
  Lines        3162    3298     +136     
=========================================
+ Hits          251     273      +22     
- Misses       2911    3025     +114     
Files Changed Coverage Δ
ext/SymPySymbolicsExt.jl 0.00% <0.00%> (ø)

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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

Successfully merging this pull request may close these issues.

5 participants