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

split to_sympy from BaseElement #888

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

split to_sympy from BaseElement #888

wants to merge 1 commit into from

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Jul 22, 2023

This PR is a proposal to disentangle Sympy conversions from the core. This allows to eliminate some local imports, and localizes a little bit more where Sympy is used.

@rocky
Copy link
Member

rocky commented Jul 24, 2023

I have mixed feeling on this. I don't think there is a problem per se in turing a method call into a function call. But in thinking about it, I wonder if this is really what we want or need.

If we want to support multiple different symbolic manipulation engines, I don't see how this moves towards that. Or rather I can see an equal amount of changes in the future when there is a second engine added to change to_sympy() to something else.

The main conceptual change as best as I can tell would be to change the name to to_sympy() to to_symbolic() or some other kind of generic name. And then to_symbolic() would decide which particular symbolic engine to use.

Right now though when we only have one symbolic engine doing this would basically add an extra level of indirection everywhere. And as mentioned in #880 it is possible or even likely that adding another symbolic engine will already have routines to follow SymPy. My understanding is that sympyengine.py works this way.

So for now I think we should defer on this until discussing and deciding on the bigger picture. And then it will be clearer if this kind of change moves in that direction or if there is some other kind of change that works better.

@mmatera
Copy link
Contributor Author

mmatera commented Jul 24, 2023

I have mixed feeling on this. I don't think there is a problem per se in turing a method call into a function call. But in thinking about it, I wonder if this is really what we want or need.

The problem that I see is that the data structures (Symbols, Expressions) are linked to sympy. So, if we want to remove sympy, these classes would have useless code. Moreover, being methods, we have to deal with these local imports because of the circular references. The first goal of this was disentangle that.

If we want to support multiple different symbolic manipulation engines, I don't see how this moves towards that. Or rather I can see an equal amount of changes in the future when there is a second engine added to change to_sympy() to something else.

If the basic data structures do not depend on sympy, I envisioned a way to be more agnostic regarding the symbolic engine: just build a package implementing the builtins that this symbolic engines support, and overload the basic one. My idea is similar to how I had implemented Integrate and FindMinimum, with a native basic implementation, and the option of using an implementation based on scipy.

The main conceptual change as best as I can tell would be to change the name to to_sympy() to to_symbolic() or some other kind of generic name. And then to_symbolic() would decide which particular symbolic engine to use.

In my opinion, to_sympy is OK, as far as it is something that lives in an optional module.

Right now though when we only have one symbolic engine doing this would basically add an extra level of indirection everywhere. And as mentioned in #880 it is possible or even likely that adding another symbolic engine will already have routines to follow SymPy. My understanding is that sympyengine.py works this way.

The problem is that probably we need different routines of conversion for sympyengine. We can then implement basic arithmetic, integrators, or analytical solvers in one or other backends, but this would imply having specific eval methods for each built-in.

So for now I think we should defer on this until discussing and deciding on the bigger picture. And then it will be clearer if this kind of change moves in that direction or if there is some other kind of change that works better.

The reason to dig a little bit into this was to have an option regarding how I implemented the changes to handle arithmetic. What I did is write a module that as far as I could, does not rely on Sympy to do basic arithmetic, so I can choose the canonical form in a closer form to the one used in WMA. I wanted also to try your idea of simply working out the back and forward conversion. Then I realize that even simple changes like using lru_cache over to_sympy and from_sympy are not possible with the current implementation. This is just a step to disentangling things. Then we can decide if we want to continue removing the to_sympy methods from the other atoms, or if we want to reintroduce them in Symbol and Expression, without breaking anything (because if they are available, they are used instead of the one defined in mathics.core.conver.sympy).

@mmatera mmatera mentioned this pull request Jul 24, 2023
@rocky
Copy link
Member

rocky commented Jul 30, 2023

Overall, while this might be a nice thing to do in the long run, nobody asked for it, the need is not that great either, and I don't think there is a clear overall plan. We have seen many times before where, "I think this might be the first step" where the overall plan was not worked out has been wrong. For example, recall the thing about changing all operator in to be BinaryOperator some specific WMA character code and that all turned out to misguided?

Right now this is a distraction and will be yet another kind of thing that is implemented halfway or less. (Hey, why not instead finish one of the unfinished items rather than starting something new like getting all of the tests that should be done in pytest moved there?)

There is an effort to make sympyengine's interface in Python be compatible with SymPy.

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.

2 participants