Alternative assignment rework #1442
Conversation
| "lhs_ = rhs_" | ||
|
|
||
| def eval(self, expr, evaluation): | ||
| "Pattern[expr, _ = _]" |
There was a problem hiding this comment.
See #1440 for how to do this without introducing Pattern[expr, ...
There was a problem hiding this comment.
On further inspection, we don't use expr here at all. So the original seems better to me.
There was a problem hiding this comment.
Ok, let's return to the previous pattern.
|
I think this is much better. I really like that we are starting to add tests for this. But more on that some other time. I would like to spend some time going over this in more detail. This is good and important work that you are doing in these PRs. Although we might not get everything right on the first round, I'd like to understand what's going on and make sure we don't have to do stuff that causes problems should we decide to revise. |
Ok, got it. I've suggested changes to #1441. Let's get them merged into master first. Do you want to make the changes, or should I make changes and put in a PR to go into #1441? |
| ) -> Optional[BaseElement]: | ||
| "lhs_ ^= rhs_" | ||
| def eval(self, expr: BaseElement, evaluation: Evaluation) -> Optional[BaseElement]: | ||
| "Pattern[expr, _ ^=_]" |
There was a problem hiding this comment.
I've been thinking about the use of Pattern[expr] to create a placeholder for expr as opposed to using something like Expression() or to_expression() and I don't see using Pattern[] here as an improvement, even in those cases where get_eval_Expression() can't be used. (More on get_eval_Expression() below).
Previously, I mentioned the fact that now the docstring or pattern is a little more complicated and harder to understand the intent — capturing is put first and foremost, while everything else follows on the line.
It was claimed that this was somehow also faster. And here, I am not so sure...
First, upon loading, there is code to read the docstring and process it. This is up-front work, whether or not the builtin is ever used. Second, on each evaluation, there is code to bind this variable. So here the question is about whether that work is less than the work done by Expression or to_expression(). Well, if this is done only to provide a more complete error message, then one expects that most of the time and in those cases where it matters most, doing lazy expression creation is also a time win.
Of course, this concern to me is a secondary concern after the misdirection aspect of adding Patter[expr] in a docstring describing the matching pattern, which causes a function to get invoked.
Now let me come back to get_eval_Expression() and how that is different from Expression(). In PR #1446, I made a pass over the Mathics3 Python code to convert Expression() and to_expression() into get_eval_Expression(). Here is what I learned from this.
One difference that can occur in the form of the expression. get_eval_Expression() shows the expression closer to how it was entered, whereas using Expression(), you can tweak this however you want to. But typically, the expression will reflect the reordering of parameters and canonicalization.
This kind of thing is most apparent in algebraic expressions.
The other "difference" is that Cython can have trouble introspecting the call stack. I guess this has to do with its "optimization". Personally, I think we should ditch Cython run over Mathics3 modules, unconditionally. As best as I can tell, it is not giving us any speed advantage nowadays. As for using it as a type checker, I think nowadays the other type checkers, like Mypy, are much better.
There was a problem hiding this comment.
I've been thinking about the use of
Pattern[expr]to create a placeholder forexpras opposed to using something likeExpression()orto_expression()and I don't see usingPattern[]here as an improvement, even in those cases whereget_eval_Expression()can't be used. (More onget_eval_Expression()below).
OK, in any case, it was not the main goal of this PR, but I found the chance to check if this work and see if there was some consensus to change in that direction.
Previously, I mentioned the fact that now the docstring or pattern is a little more complicated and harder to understand the intent — capturing is put first and foremost, while everything else follows on the line.
It was claimed that this was somehow also faster. And here, I am not so sure...
First, upon loading, there is code to read the docstring and process it. This is up-front work, whether or not the builtin is ever used.
OK, but this overhead happens at load time, and probably can be avoided if we compile and store the rules.
Second, on each evaluation, there is code to bind this variable. So here the question is about whether that work is less than the work done by
Expressionorto_expression(). Well, if this is done only to provide a more complete error message, then one expects that most of the time and in those cases where it matters most, doing lazy expression creation is also a time win.
Actually, what would be faster would be something like expr:Set[___], just because to match the pattern -when there are no other rules- would be faster than matching Set[x_,y_]. In any case, this would impact performance only if we use several assignment statements in a loop.
Of course, this concern to me is a secondary concern after the misdirection aspect of adding
Patter[expr]in a docstring describing the matching pattern, which causes a function to get invoked.Now let me come back to
get_eval_Expression()and how that is different fromExpression(). In PR #1446, I made a pass over the Mathics3 Python code to convertExpression()andto_expression()intoget_eval_Expression(). Here is what I learned from this.One difference that can occur in the form of the expression.
get_eval_Expression()shows the expression closer to how it was entered, whereas usingExpression(), you can tweak this however you want to. But typically, the expression will reflect the reordering of parameters and canonicalization.
An even simpler way to find the expression that is actually evaluated would be to store it in another attribute of the Evaluation object. This does not require any call to introspection functions or loops: just access to a Python variable. In any case, I think the proposal of using get_eval_Expression() is good in connection with the current development of the debugger API.
This kind of thing is most apparent in algebraic expressions.
The other "difference" is that Cython can have trouble introspecting the call stack. I guess this has to do with its "optimization". Personally, I think we should ditch Cython run over Mathics3 modules, unconditionally. As best as I can tell, it is not giving us any speed advantage nowadays. As for using it as a type checker, I think nowadays the other type checkers, like Mypy, are much better.
This is another problem. In any case, this is a different problem. By now, since the goal is to improve mathics.eval.assignment module, I removed the changes in mathics.builtin except for the ones required to pass the tests.
There was a problem hiding this comment.
An even simpler way to find the expression that is actually evaluated would be to store it in another attribute of the
Evaluationobject. This does not require any call to introspection functions or loops: just access to a Python variable. In any case, I think the proposal of usingget_eval_Expression()is good in connection with the current development of the debugger API.
See #1450 as an example of this approach
| @@ -173,6 +173,11 @@ def match(self, expression: Expression, pattern_context: dict): | |||
| # yield new_vars_dict, rest | |||
| self.pattern.match(expression, pattern_context) | |||
|
|
|||
| def get_sort_key(self, pattern_sort=True): | |||
| if not pattern_sort: | |||
There was a problem hiding this comment.
Instead of parameter patten_sort, why not define function get_pattern_sort_key()?
It would be nice to add a docstring that explains that how each sort is used, e.g. get_sort_key is used in ordering of expression elements. While get_pattern_sort_key is used to prioritize which rule to select.
There was a problem hiding this comment.
|
I don't like the invented term "focus" to mean lhs without conditions, if that's what it is. Please change to something that conveys the meaning better. Thanks. |
target? main_expression? |
How about |
without condition, holdpattern, exception,... stripped_lhs? bare_lhs? |
I don't understand. Is it correct to say this is the left-hand side with Condition expressions removed? If so, (rms in GNU Emacs once used the French "sans" for "without"; it is has a slightly culinary meaning in English as well, but Spanish "sin" would be kind of funny appropriate; but it would be confusing too since "sin" is an English word.) |
This is why I think that names like |
The common programming language term for this is: reference, and here, left-hand-side reference.
The problem with "focus" as used here is that there are numerous ways this word could be interpreted and used as an analogy. Like starting with whether the meaning is used as a noun or a verb. In places where you want to appropriate a term from one domain to another, the term should have a very clear, specific, and unambiguous meaning. For example, when Robert Tarjan took the term "amortized" from finance and applied it to algorithms, it is very clear what that means.
Okay, fair enough. How about |
|
Thanks for your patience and tolerance. |
Sure. Thank you for your careful revision! |
Co-authored-by: R. Bernstein <[email protected]>
Co-authored-by: R. Bernstein <[email protected]>
|
OK, let's go on with this, and iterate |
This is an alternative, less disruptive implementation for the assignment refactor.