mathics.eval #630
mathics.eval #630
Conversation
* adding the module mathics.eval * mathics.core.evaluators-> mathics.eval.nevaluator * formatter.format_element and related routines -> mathics.eval.makeboxes
| @@ -571,7 +571,7 @@ class Power(BinaryOperator, _MPMathFunction): | |||
| 'Infix[{HoldForm[x], HoldForm[y]}, "^", 590, Right]' | |||
| ), | |||
| ("", "x_ ^ y_"): ( | |||
| "PrecedenceForm[Superscript[OuterPrecedenceForm[HoldForm[x], 590]," | |||
| "PrecedenceForm[Superscript[PrecedenceForm[HoldForm[x], 590]," | |||
There was a problem hiding this comment.
This is something that I guess I have corrected several times...
There was a problem hiding this comment.
@mmatera - sorry. .Then let us make sure this gets in!
There was a problem hiding this comment.
PS. Another way to make sure this gets in is to create a PR with just this change and it will go through faster.
mmatera
force-pushed
the
mathics_eval_module
branch
from
081f603 to
a1bb326
Compare
mathics/core/definitions.py
Outdated
| @@ -107,7 +107,7 @@ def __init__( | |||
| "Global`", | |||
| ) | |||
|
|
|||
| from mathics.builtin.pymathics import ( | |||
| from mathics.eval.pymathics import ( | |||
There was a problem hiding this comment.
It was my understanding that the idea behind eval was to transition to a model where we could run a sequence of instructions where each instruction corresponds to an eval function right now.
I am not opposed to moving around the functions that load modules or separating pymathics things, but I don't get why the natural place for this is in mathics.eval
There was a problem hiding this comment.
I put it here because is what if follows from evaluating an expression like LoadModule[module]. In any case, this was in mathics.builtin.pymathics and it does not belong there. Maybe it belongs to mathics.core.pymathics.
There was a problem hiding this comment.
I put it here because is what if follows from evaluating an expression like
LoadModule[module]. In any case, this was inmathics.builtin.pymathicsand it does not belong there. Maybe it belongs tomathics.core.pymathics.
Ok. Sure mathics.core or somewhere else is fine. Probably some of the things in as yet not finished mathics.builtin.system_init belong in core as well.
mathics/eval/pymathics.py
Outdated
| @@ -0,0 +1,106 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
I had a comment about this in the other PR that I guess comes after this. Please see that.
mmatera
force-pushed
the
mathics_eval_module
branch
from
7454458 to
5caa0c3
Compare
|
LGTM. (I assume this will continue pass CI) |
| # do_format_* | ||
|
|
||
|
|
||
| def do_format( |
There was a problem hiding this comment.
The do_format... names were poorly selected and now that they have moved here moreso.
Better would be just to drop the do_ which doesn't add or do anything for clarity. Things in the eval module are automatic "do-ers". "eval" is a active verb and "format" is also an active verb.
There was a problem hiding this comment.
Let me explain the do_ convention as I understand it. When mixed with a number of other methods, a method marked do_ is the top-level function to call which presumably calls the other methods in the class that don't have do_. Similarly sometimes it is used as a function outside the class to kick off methods inside the class.
Neither of these situations now applies here.
In general though, if one is using this a lot, it is an indication that the code organization may need to be reworked as was done in this PR.
There was a problem hiding this comment.
Regarding this, the plan is that this module is going to be reformulated soon. I didn't change the names just to make this PR a little bit shorter and easier to review.
There was a problem hiding this comment.
Regarding this, the plan is that this module is going to be reformulated soon. I didn't change the names just to make this PR a little bit shorter and easier to review.
Ok - good to hear we are on the same page here.
I looked again to see if I understand the "plan" and I see this:
formatter.format_element and related routines -> mathics.eval.makeboxes
This doesn't seem right. eval methods should be thought of as instructions.
Boxing is something that could be written purely in Mathics. It is not on the same level as an interpreter instruction. If you want to put boxing routines in a different place, okay But it is not an "eval" thing.
Boxing "evaluation" which I think we call "formatting" and M-Expression evaluation are very different things.
There was a problem hiding this comment.
OK, I do not have a complete picture of this right now, and I think that its future form would require some discussion. However, what is clear is that this code does not belong to mathics.core.formatter and now, this behaves like a different kind of evaluation (just because it uses a different set of rules).
I will try to reimplement most of this code as a WL package, and then we can see what part belongs here, or if mathics.eval.makeboxes should be removed at the end.
Moving evaluators to mathics.eval module
If the number of files is quite large, almost all of the changes are just renaming the import modules.
One advantage of this organization is that makes it easier to avoid circular dependencies.