-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: Add Transformer class and bin_expand function #30
Conversation
CC @brocksam (not sure if you get automatically notified otherwise). I will make a separate PR to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all seems logical. I understand the need for Transformer
. Will be interested to see if any extra machinery is needed to get lambdify
to work because it'd be nice to not have to introduce further Evaluator
-like classes without good reason (my feeling is that Evaluator
should be able to cover both interpretation and printing as complexity increases).
Transformer
has got me thinking about simplification and rewriting. I'm now pondering whether it'd be possible to use (a slightly modified) Transformer
to handle the shallow simplification rewrites that we discussed yesterday, e.g. -1*(-1*sin(x)) -> sin(x)
, or whether some form of pattern matching would still be required.
I think what is needed is something like Most In [5]: N(x + sqrt(2))
Out[5]: x + 1.4142135623731 Here we can Ideally it would also handle things like evaluating a subset of args to an associative/commutative operator and recursing through operations that are undefined. SymPy's In [10]: f = Function('f')
In [11]: N(f(sqrt(2)))
Out[11]: f(sqrt(2))
In [12]: N(exp(sqrt(2)*x))
Out[12]: exp(sqrt(2)*x)
In [13]: N(sqrt(2)*x)
Out[13]: 1.4142135623731*x |
Thanks for the review! I'll merge this now and I think I'll fix the |
Yes, I've been wondering this as well. I think I always wanted a clean separation between "evaluation" and "transformation" but as discussed in my last comment it is not always possible to cleanly separate them e.g. because of partial |
I think
Bringing all numbers together at the front of an In [14]: Mul(x, 2, evaluate=False).as_coeff_Mul()
Out[14]: (1, x*2)
In [15]: Mul(x, 2).as_coeff_Mul()
Out[15]: (2, x) Probably many of these rules can be made generic in some sense as rules for:
The same rules could then ideally be used for In so far as possible I would like to preserve the idea of not having unnecessary automatic simplification but we should keep in mind that there are probably some good reasons that very few CAS systems take that path. |
There are several things mixed up here:
bin_expand
method toExpr
.Transformer
as an alternative toEvaluator
Evaluator
to have aneval_atom
method and renamecall
toeval_operation
.protosym.core/exceptions
module to define exception types.f(x)
insimplecas
.A precursor to adding lambdify with LLVM is having a
bin_expand
method that can turn an associative operation into a sequence of binary operations e.g.(x + y + z)
->((x + y) + z)
. This is because LLVM IR only defines binary operations. Actually it is currently difficult in protosym to create a flattenedAdd
like(x + y + z)
because noflatten
function is provided and all natural operations like+
will give unflattened expressions consisting of binary operations. An expression converted from SymPy for example will have associative operations though:Likewise when parsing is added I would want
parse('x + y + z')
to give a flattenedAdd
.When writing the
bin_expand
function I realised that it is quite awkward to do withEvaluator
becauseEvaluator
requires defining rules for all atoms and heads. In the case ofbin_expand
we only want to modifyAdd
andMul
and leave all other expressions unchanged but we don't want to have to list rules for all of the expression types that we don't want to change. For this I added aTransformer
class as a subclass ofEvaluator
, ATransformer
is for converting aTreeExpr
nto anotherTreeExpr
and allows any atoms or heads without rules to pass through unmodified. See the tests and doctests for examples comparing this withEvaluator
.I also discovered that in simplecas this fails and added an XFAIL test for it:
This is somewhat similar to the
bin_expand/Evaluator
issue. Basically there are printing rules forf
as an atom:However when
f
is a head as inf(x)
the evaluator expects a rule for each head like a rule forsin(...)
andrule for
cos(...)`. There needs to be a way to give a default rule for the case when there is not a rule for the given head.I think that it is always good practice to have an exceptions module that downstream code can import from and look in to see all of the possible exceptions so I've added that. Currently we have
Perhaps the other exceptions raised should be changed to use something other than
TypeError
andNotImplementedError
. I think it is generally good not to reuse exception types that are already used in other places because then someone trying to catch exceptions can catch the wrong one. Ideally an exception class should be used to identify a clear scope in the code for where the exception comes from if it is caught anywhere.