Rule code revision (part 1)#1863
Conversation
| of an expression. The tuple is ultimately compared lexicographically. | ||
| """ | ||
| # FIXME: check if this makes sense: | ||
| return tuple((self.system, self.pattern.element_order)) |
There was a problem hiding this comment.
@mmatera: Why is a system parameter needed in a Rule? It appears to distinguish pymathics modules from Mathics-Core code. But why do we care about this?
Recall that recently some Mathics-Core code was moved to a PyModule. And we promised that the behavior would be the same. Adding this distinction for a rule seems to violate that principle.
There was a problem hiding this comment.
This parameter was there before the introduction of modules. At that point, I just mimic the behavior of built-in symbols.
There was a problem hiding this comment.
Ok. Thanks for the information. Are you okay with removing this parameter? As far as I can tell, it just complicates things and makes a distinction where there should be none. (Eventually, we might be able to get a "location" of a rule, which would also indicate where this comes from, not that I think that there is a reason to know this.)
And more generally, are you okay with the rest of this PR?
1916644 to
ee207ad
Compare
|
For context, I started down this rabbit hole because I was l looking to add InformationData so we could show Information output better in front-ends. InformationData is really a kind of Association, and an Association is described as a list of Rules, although when this happens, this would be better represented as a dictionary. It is sad that a decade out, we still need to revise the code at the lowest levels. |
| return _python_function_arguments(f) | ||
|
|
||
|
|
||
| def is_rule(element: Any, include_delayed: bool = True) -> bool: |
There was a problem hiding this comment.
Why we would like to distinguish RewriteRule's from FunctionApplyRules?
There was a problem hiding this comment.
I'm glad you asked.
In the Wolfram Language, the word "Rule" can be thought of as either being overloaded for different concepts or as a very generic term.
Associations and Options contain "Rules" to indicate each name-value pair, for example. To me, this feels like a very different thing than a rewrite rule, which rewrites an expression as part of an evaluation of an expression.
And it looks to me like these kinds of rules are different:
Consider this situation where we use a "rule" inside an association:
In[1]:= y = <|10 -> "foo" |>
Out[1]= <|10 -> foo|>
In[2]:= y[10]
Out[2]= fooHere, the rule acts as a key-value pair that is meaningful in terms of the Association collection, but not so much as a single key-value pair.
Furthermore, in contrast to a rewrite rule, pattern matching doesn't seem to have an effect:
In[3]:= z = <|x_ -> "bar"|>
Out[3]= <|x_ -> "bar"|>
In[4]:= z[20]
Out[4]= Missing[KeyAbsent, 20]Rules are also mentioned inside OptionsPattern.
And again, this functions like a key-value pair in a collection variable rather than as a rewrite rule that kicks in during expression evaluation.
The name RewriteRule on the class is then to try to distinguish that we are talking about the kind of "rule" that happens during evaluation and explicit pattern-match situations as opposed to key-value pair situations that happen during Association and Option lookup.
As for coding, I think we need to have different internal structures for the two kinds of concepts.
We want to use Python dictionaries to hold the Option and Association dictionaries. (Actually, we already seem to do this for Options).
Right now, though, I don't fully understand how to structure things to make this work. I know, though, that the code we have is a bit convoluted (hard to understand) and inefficient in whatever it is doing by taking the kitchen-sink approach to implementing "Rule".
There was a problem hiding this comment.
Let me also add that "Rule" is a parsing concept; -> is an infix operator for a "Rule". However, semantically, it seems like there are at least those two distinctions mentioned above (and there may be more).
There was a problem hiding this comment.
Why we would like to distinguish
RewriteRule's fromFunctionApplyRules?
I realize I didn't even answer the question! I answered the question why we would distinguish RewriteRule from Rule. As for RewriteRule and FunctionApplyRule, they are, without question, different things. One has a Python method as a target, which is not user-programmable. On the other hand RewriteRules, users can write. Their implementations are different too. Combining these into one routine, I think, is a bad programming design idea.
The Mathics3 code base suffers from trying to do things that vary a bit, and combine them into a single overly complicated routine. We had this with import for example. (And in fact we still have import-related code that one of the linters complains is too complicated.)
There was a problem hiding this comment.
Indeed, a Rule[...]/RuleDelayed[] expression has different meanings according to the context. If they are inside an Association expression, they are handled as pairs of key/value, while in evaluation, and inside a Dispatch expression, they are treated as rewrite rules.
Rules are also mentioned inside OptionsPattern.
And again, this functions like a key-value pair in a collection variable rather than as a rewrite rule that kicks in during expression evaluation.
It is also true.
The name RewriteRule on the class is then to try to distinguish that we are talking about the kind of "rule" that happens during evaluation and explicit pattern-match situations as opposed to key-value pair situations that happen during Association and Option lookup.
Elements in an association are not "rules", but it seems that for WR guys, it looks appealing to use the same symbol for rules and key/val pairs, probably for the reason you mention below (reuse -> / :> operators).
In any case, my question was about why to distinguish RewriteRules and ApplyFunctionRules, not about this other homonimous structure.
As for coding, I think we need to have different internal structures for the two kinds of concepts. We want to use Python dictionaries to hold the Option and Association dictionaries. (Actually, we already seem to do this for Options).
Right now, though, I don't fully understand how to structure things to make this work. I know, though, that the code we have is a bit convoluted (hard to understand) and inefficient in whatever it is doing by taking the kitchen-sink approach to implementing "Rule".
Notice also that when Rule was implemented in Mathics, it was nothing to do with Associations, which at that time was not implemented.
There was a problem hiding this comment.
I realize I didn't even answer the question! I answered the question why we would distinguish RewriteRule from Rule. As for RewriteRule and FunctionApplyRule, they are, without question, different things. One has a Python method as a target, which is not user-programmable. On the other hand RewriteRules, users can write. Their implementations are different too. Combining these into one routine, I think, is a bad programming design idea.
What BaseRule subclasses represent is something that takes an expression, tries to match it, and if it matches, generates a new expression + do make side effects. This applies both to Rewrite rules and to ApplyFunction rules. The difference comes in how the rule is defined. In the first case, typically (but not always) the rule is defined in terms of a "template" expression, and the rewrite just consists of replacing symbols with subexpressions of the original expression. In some cases, (for example, when "Condition" subexpressions appear) the subexpressions are evaluated to control if the rule matches. Some of these rules could then be "compiled" as ApplyFunctionRules.
There was a problem hiding this comment.
I looked at the implementation again, and although this PR tries to make them look more similar by adding a common rhs method, the code for the two is largely dissimilar. And this is all historical: they've always been separate.
Yes, the two are similar in spirit, and that's why they share the same base class. So, all of this is as I think it should be.
Note the different implementations of rhs because one uses the attribute replace, while the other uses an attribute called function.
| self.location: Optional[Callable] = None | ||
| self.pattern = BasePattern.create( | ||
| pattern, attributes=attributes, evaluation=evaluation | ||
| pattern, attributes=attributes, evaluation=None |
There was a problem hiding this comment.
Here, the evaluation object was passed to the constructor to provide access to symbol attributes, which modify the properties of the rules at build time.
There was a problem hiding this comment.
This is something that's been nagging me.
It feels like we have the tail wagging the dog.
Throwing in an entire evaluation for each rule so that pickling works is an indicator to me that we're probably doing something wrong. We haven't noticed the gross inefficiency because we haven't been pickling much.
The use case where a Pattern/Rewrite rule has Expression Attributes seems like a very special corner case and is not used much, rather than something fundamental to pattern matching, rewrite rule evaluation, or key-value pair lookup.
And on top of that, I think somewhere else, it has already been noted that the attribute computation is wrong because it does not take into account nested patterns.
Whatever code is relying on evaluation context residing inside a Rewrite Rule should be fixed so that it doesn't work that way, but instead takes an evaluation parameter.
Rewrite Rules are the only rules where this kind of thing happens, right?
There was a problem hiding this comment.
Again, the evaluation object is used to "compile" the rule. It is not necessary to preserve it as an object member. When a rule is defined for an "Orderless" or "OneIdentity" expression, that attribute is used to check the match. To know that, we need (at "compilation" time) the current attributes.
Consider, for instance, this example
(*Set F as Orderless*)
In[1]:= SetAttributes[F,{Orderless}]
In[2]:= ?? F
Out[2]= Global`F
Attributes[F]={Orderless}
(*Define a rule*)
In[3]:= F[x_Symbol, t_Integer]:={x,t}
(*This matches the rule as it is*)
In[4]:= F[a,1]
Out[4]= {a, 1}
(*This matches because of the orderless attribute*)
In[5]:= F[1,a]
Out[5]= {a, 1}
(*Now, the attribute is "unset" from F*)
In[6]:= Attributes[F]= {}
(*However, the created rule still remembers the attributes at the "compilation" time *)
In[7]:= F[1,a]
Out[7]= {a, 1}
There was a problem hiding this comment.
Again, the evaluation object is used to "compile" the rule.
I am not sure what you mean by "compile," or whether it means the same thing to me that you are thinking of.
Mathics3 programs are scanned, parsed, and interpreted. We don't do any "compiling" per se. And there is vagueness about whether we are talking about the interpretation done for the LHS of SetDelayed involving F, or the interpretation for the function call, F.
When the program evaluates the parsed expression for the call F[a,1], from your example, that's when the attributes of F should be looked up. I do not see that the data structure for the RewriteRule needs to have attribute or evaluation information stored inside it when the SetDelayed is interpreted.
In Part 2 PR (after the future reduced Part 1, which keeps things as it is currently), I'll suggest how this might be done.
But even as things are, note that evaluations and attributes are needed only for RewriteRule, not BaseRule (or a future KeyValueRule). So those parameters should not have been put in BaseRule. (The mechanics of how to do this now I am fuzzy on because I sometimes see lint messages about violating the Liskov inheritance principle or something like that.)
There was a problem hiding this comment.
With "compile" I was thinking in something like Python "compiled" regular expressions: just build an optimized representation of one or more pattern expressions to be applied faster. In Mathics, to apply rules, we need to convert a Rule[...] expression into a (Rewrite)Rule object. This step requires looking at the current state of the evaluation object and use the attributes to determine specific match criteria.
On the other hand, I guess that the notion of "interpretation" in WMA is a little bit different from the notion in other "interpreted languages" like Python, javascript, or Basic. What we have here are replacement rules, applied in a certain sequence. In Python, a block of lines are interpreted (compiled on the fly) into some pseudocode, and then the input is procesed with that pseudocode, right?
Regarding this:
When the program evaluates the parsed expression for the call F[a,1], from your example, that's when the attributes of F should be looked up. I do not see that the data structure for the RewriteRule needs to have attribute or evaluation information stored inside it when the SetDelayed is interpreted.
what the example shows is that the attributes are attached to the rule at the time it is created. Then these stored attributes are used, instead of the attributes attached to the symbol at the time of the rule application. That is what I tried to show with the example.
In Part 2 PR (after the future reduced Part 1, which keeps things as it is currently), I'll suggest how this might be done.
OK
But even as things are, note that evaluations and attributes are needed only for RewriteRule, not BaseRule (or a future KeyValueRule). So those parameters should not have been put in BaseRule. (The mechanics of how to do this now I am fuzzy on because I sometimes see lint messages about violating the Liskov inheritance principle or something like that.)
Again, I think that making a "KeyValueRule" that inherits from BaseRule would increase the confusion. "KeyValue" rules are just elements of an Association, which does not have anything to do with transformation/replacement rules. A replacement rule has a pattern and an apply method. "KeyValue" rules would have a key and a value, and no "apply" method.
On the other hand, both FunctionApplyRule and Rewrite rules require the knowledge about attributes at the initialization time (compare builtin rules attached to Plus, which are Orderless and BesselJ which are not).
There was a problem hiding this comment.
With "compile" I was thinking in something like Python "compiled" regular expressions: just build an optimized representation of one or more pattern expressions to be applied faster. In Mathics, to apply rules, we need to convert a
Rule[...]expression into a(Rewrite)Ruleobject. This step requires looking at the current state of the evaluation object and use the attributes to determine specific match criteria.On the other hand, I guess that the notion of "interpretation" in WMA is a little bit different from the notion in other "interpreted languages" like Python, JavaScript, or Basic.
Python sort of, Basic no, not usually.
In Python, a block of lines are interpreted (compiled on the fly) into some pseudocode, and then the input is procesed with that pseudocode, right?
Like Mathics3, the entire text is scanned and parsed, and a tree representation is created. (In Python it is called an AST, in Mathics3 and WL it is an M-expression.) In all Pythons, the AST is the same. In most Pythons, the AST is then compiled/translated into high-level machine-independent instructions called bytecode (in Graal it's JVM bytecode) which is then often written to disk (but it doesn't have to). In CPython, the most common Python, bytecode instructions have traditionally been interpreted. I'd like to say analogous to Mathics3, but Mathics3 follows more of the kind of thing done in Perl5, Korn Shell, and Lisp when handling its S-Expressions.
Some Pythons JIT the bytecode; some convert it into WASM; how WASM is run is another discussion.
Regarding this:
When the program evaluates the parsed expression for the call F[a,1], from your example, that's when the attributes of F should be looked up. I do not see that the data structure for the RewriteRule needs to have attribute or evaluation information stored inside it when the SetDelayed is interpreted.
what the example shows is that the attributes are attached to the rule at the time it is created. Then these stored attributes are used, instead of the attributes attached to the symbol at the time of the rule application. That is what I tried to show with the example.
You mean at the time F[a, 1] is called/evalutated rather than at the time F[x_Symbol, t_Integer] is defined via a SetDelayed evaluation. Right?
I will leave open how to implement this for later. But that evaluation parameter is not used in any of the methods in that class. And removing that parameter altogether did not break any of our existing tests.
Including the entire evaluation object for pickling is most likely a bad idea.
In Part 2 PR (after the future reduced Part 1, which keeps things as it is currently), I'll suggest how this might be done.
OK
But even as things are, note that evaluations and attributes are needed only for RewriteRule, not BaseRule (or a future KeyValueRule). So those parameters should not have been put in BaseRule. (The mechanics of how to do this now I am fuzzy on because I sometimes see lint messages about violating the Liskov inheritance principle or something like that.)
Again, I think that making a "KeyValueRule" that inherits from BaseRule would increase the confusion. "KeyValue" rules are just elements of an Association, which does not have anything to do with transformation/replacement rules. A replacement rule has a pattern and an apply method. "KeyValue" rules would have a key and a value, and no "apply" method.
I never thought it would be part of BaseRule. It would be its own standalone class. However, RewriteRule creation would trigger the creation of that kind of object when it is told to do so from being called inside Association or Option, or when it so happens that the LHS is not a "pattern" or rather the degenerate constant pattern, i.e nothing to match other than the entire string as it appears.
The reason we would need this kind of object is more for bookkeeping and conversion back and forth rather than as something that is used in evaluation to find the value associated with a key. (For that, the Python dictionary would work.)
On the other hand, both FunctionApplyRule and Rewrite rules require the knowledge about attributes at the initialization time (compare builtin rules attached to
Plus, which areOrderlessandBesselJwhich are not).
I was under the impression that FunctionApplyRule is only used by Python-backed builtin functions (including those that appear via Mathics3 Python Modules).
There was a problem hiding this comment.
bytecode) which is then often written to disk (but it doesn't have to). In CPython, the most common Python, bytecode instructions have traditionally been interpreted. I'd like to say analogous to Mathics3, but Mathics3 follows more of the kind of thing done in Perl5, Korn Shell, and Lisp when handling its S-Expressions.
Some Pythons JIT the bytecode; some convert it into WASM; how WASM is run is another discussion.
OK, thanks for the information. What I just tried to mean is that differently from languages like Python, WL does not implement sequences of "instructions", but replacement rules.
Regarding this:
When the program evaluates the parsed expression for the call F[a,1], from your example, that's when the attributes of F should be looked up. I do not see that the data structure for the RewriteRule needs to have attribute or evaluation information stored inside it when the SetDelayed is interpreted.
SetDelayed build and store rules in the Definitions dictionaries. When it built the rules, it passes the evaluation object, that is needed to set the attributes attributes of the pattern class. It must be done recursively, because each sub-pattern needs to consult the Definitions object to load the corresponding attributes.
When attributes are not given at initialization time, the apply method loads it from the current evaluation object (apply passes it).
what the example shows is that the attributes are attached to the rule at the time it is created. Then these stored attributes are used, instead of the attributes attached to the symbol at the time of the rule application. That is what I tried to show with the example.
You mean at the time
F[a, 1]is called/evalutated rather than at the timeF[x_Symbol, t_Integer]is defined via a SetDelayed evaluation. Right?I will leave open how to implement this for later. But that evaluation parameter is not used in any of the methods in that class. And removing that parameter altogether did not break any of our existing tests.
Including the entire evaluation object for pickling is most likely a bad idea.
Evaluation object should not be pickled. I fully agree with this.
In Part 2 PR (after the future reduced Part 1, which keeps things as it is currently), I'll suggest how this might be done.
OK
But even as things are, note that evaluations and attributes are needed only for RewriteRule, not BaseRule (or a future KeyValueRule). So those parameters should not have been put in BaseRule. (The mechanics of how to do this now I am fuzzy on because I sometimes see lint messages about violating the Liskov inheritance principle or something like that.)
Again, I think that making a "KeyValueRule" that inherits from BaseRule would increase the confusion. "KeyValue" rules are just elements of an Association, which does not have anything to do with transformation/replacement rules. A replacement rule has a pattern and an apply method. "KeyValue" rules would have a key and a value, and no "apply" method.
I never thought it would be part of BaseRule. It would be its own standalone class. However, RewriteRule creation would trigger the creation of that kind of object when it is told to do so from being called inside Association or Option, or when it so happens that the LHS is not a "pattern" or rather the degenerate constant pattern, i.e nothing to match other than the entire string as it appears.
The reason we would need this kind of object is more for bookkeeping and conversion back and forth rather than as something that is used in evaluation to find the value associated with a key. (For that, the Python dictionary would work.)
OK. Just that in my mind, Associations would be stored as simple Python dictionaries, with expressions used as keys. But if you think it is useful, go ahead.
On the other hand, both FunctionApplyRule and Rewrite rules require the knowledge about attributes at the initialization time (compare builtin rules attached to
Plus, which areOrderlessandBesselJwhich are not).I was under the impression that FunctionApplyRule is only used by Python-backed builtin functions (including those that appear via Mathics3 Python Modules).
Right now, that is the case. But we could implement some simple rules in a different way, by using FunctionApplyRule representation.
OK with this change.
Not sure why we want to distinguish ApplyFunctionRules from RewriteRules.
The evaluation object is used at build time to determine certain attributes of the symbols. So, for example, if you have a rule involving a subexpression with
I think this is fine. Not sure if this breaks the canonical order at some point, but we can check.
|
51b9ba6 to
a23c418
Compare
I think what I'll do then is create a new PR that has the non-controversial parts. And then we'll consider the rest separately. There is a lot more here going on that I had not understood, and I thank you for enlightening me here on what's up. It is becoming clearer after the discussion the direction this code might take to have it work better and be clearer as to intent and implementation. I suspected there would be a lot more churn, which is why I titled this "part 1". |
Go over comments
Do we really need this? If so, why?
Add rhs property
a23c418 to
82bfd6c
Compare
|
Superseded by #1866 |
2 participants
Class
mathics.core.rules.Ruleis nowmathics.core.RewriteRule.Add a
is_rule()function to replaceexpr.has_form(...)Remove the evaluation parameter in BaseRule. Note this makes it impossible to make Rules immutable and serializable on disk.
Remove the "system" parameter on Rule creation.
Go over docstring comments.