Adding pytests for from_sympy / to_sympy. #828
Conversation
…builtin` to `mathics.core.convert.sympy`. * move the implementation of `Expression.to_sympy` and `Symbol.to_sympy` to `mathics.core.convert.sympy`. * adding pytests for `from_sympy` and `to_sympy`.
|
At first glance, looks like a welcome improvement. Would like to try this out and go over in detail later. |
mathics/core/expression.py
Outdated
|
|
||
|
|
||
| # These are constant expressions used in several places. | ||
| # TODO: See if the is a better place to declare them. |
There was a problem hiding this comment.
There is no reason this can't be put in its own separate module as was done between the symbols and systemsymbols module .
More smaller focused modules is better than one larger sprawling module
There was a problem hiding this comment.
OK, any suggestions for the name of such a module?
There was a problem hiding this comment.
These all seem to be symbolic constants.
So mathics.core.symbolic_constants.
There was a problem hiding this comment.
At one time I had thought that we might set elements_properties, but that is not needed - once set on the first use will no longer need to get set again.
(And computing the properties for any of these is very quick.)
We might consider if it is worth it to turn this create an ExpressionConstant class which has a special and simple evaluate method (and other custom and simple methods like that).
There was a problem hiding this comment.
OK, maybe I can make a separate PR with these objects and module, and then reformulate over it.
There was a problem hiding this comment.
I think this was solved in #831
This comes from #828. Here a new module is added to contain the expression constants of the form `DirectedInfinity[...]`.
|
LGTM |
This PR adds some basic tests to check the consistency in the behavior of
from_sympy. When I was writing the tests, I found some issues in the behavior of these functions, and also some failures in the modularity. So, this PRmathics_to_sympyandsympy_to_mathicsdicts frommathics.builtintomathics.core.convert.sympy. This allows for avoiding some local imports.to_numeric_sympy_argswas moved frommathics.core.convert.expressiontomathics.core.convert.sympy, which again helps to reduce local imports.Expression.to_sympyandSymbol.to_sympytomathics.core.convert.sympy.MATHICS_*INFINITYand numbersMATHICS_COMPLEX_Iwere included inmathics.core.expressionto avoid local duplication.Symbol.to_sympyby using a dictionary to translate some predefined symbols in SymPy, likepi,E,GoldenRatioorI.from_sympyandto_sympy.In the code, it is also a proposal for a rewriting of
from_sympyusing dictionaries, which could help to get a clearer code.