-
-
Notifications
You must be signed in to change notification settings - Fork 61
adding evaluate_without_side_effects for helping compile expressions.
#1539
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
base: master
Are you sure you want to change the base?
Conversation
| self.var = var | ||
|
|
||
|
|
||
| def evaluate_without_side_effects( |
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.
Not sure if this is the right place for this function.
| for name, defin in SIDE_EFFECT_BUILTINS.items(): | ||
| # Change the definition by a temporal definition setting | ||
| # just the name and the attributes. | ||
| definitions.builtin[name] = Definition( |
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.
maybe these definitions can be stored in some place, and then each time we compile something, bring them up, instead of constructing them each time.
| = 2.18888 | ||
| Loops and variable assignments are supported usinv Python builtin "compile" function: | ||
| >> Compile[{{a, _Integer}, {b, _Integer}}, While[b != 0, {a, b} = {b, Mod[a, b]}]; a] (* GCD of a, b *) |
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 test was wrong in many ways. On the one hand, this expression shows the following warning in WMA:
Compile::argset:
The assignment to a is illegal; it is not valid to assign a value to an
argument.
Then, if we try to evaluate the compiled function, we get more errors, and an unfinished loop.
The expected result is also wrong in another way: it expects the wrong behaviour, where the expression is fully evaluated before compiling, to get a. The new test is something that actually works in WMA, and produces the expected behavior.
| """ | ||
|
|
||
| attributes = A_HOLD_ALL | A_PROTECTED | A_N_HOLD_ALL | A_READ_PROTECTED |
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.
These attributes are very important: they prevent the evaluation of the original compiled expression.
This PR adds a helper function for evaluating expressions, keeping unevaluated those expressions that could have side effects, like changing the value of a variable. This could be useful to convert (compile) expressions into Python functions.
Let' s consider the following WL code:
F[x,y]is a convoluted way to write (x+IntegerPart[y]), but I imagine more complicated cases where this kind of construction is needed.If we use the vectorized version of
Plot3D,F[x,y]is compiled to a vectorized Python function through the functionplot_compile.plot_compile(evaluation,expr, ...)tryies to convertF[x,y]into a Sympy expression. To do that, it needs to evaluate firstF[x,y]into an expression involving justx,y, and built-in symbols. In the current implementation, this is done by first evaluating the expressionexpr(expr = expr.evaluate(evaluation)), and then trying to convert it to sympy. But in the case of the example, evaluatingF[x,y]results inx, and leaves assigned the symbolawith the symbolx, which is an undesired side effect.This PR should avoid the evaluation of
Do,Set, andCompoundExpression. This makes the conversion to sympy fail, forcing the evaluation to use the standard method.In another round, we could also provide suitable conversions to SymPy for these symbols, allowing a proper vectorized implementation.