Skip to content
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

Moving eval mixin methods to the eval module #979

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Jan 26, 2024

This draft starts to move part of the implementation of the Expression.evaluation method to the mathics.eval module. Also, the fat routine rewrite_apply_eval_step() is decomposed into different subroutines. @rocky, I hope this helps with the upcoming work, post the 7.0.0 release.

@rocky
Copy link
Member

rocky commented Jan 28, 2024

While I agree that we tend to have modules and large functions and that mathics.core needs work, I have grave misgivings about this PR.

mathics.eval and mathics.core have a very specific well-defined purposes and this muddies or muddles those purposes. So let me try to describe those purposes. If we need to adjust the Mathics3 developer guide to make it clearer, please point out what needs to be specified and revised, then please let's do that.

mathics.core has the lowest level concepts or atoms (in the physics sense) that everything is built up from. So we have things like Symbols, Integers, Strings and so on. These are degenerate S-Expressions (and therefore also degenerate M-Expressions). At the next level up at the molecular level, we have "compound" M-Expressions: those things that are built up from the atomic symbols, strings, numbers, etc.

rewrite_apply_eval_step() for (compound) Expression is fat largely because this is the ugly complex semantics of general WMA Expression evaluation. This is the second part of Einstein's well known phrase "as simple as possible.." is: "but no simpler".

I do believe right now this code has a little baggage because it is improperly called from Boxing routines when instead Boxing evaluation would have a separate routine which may sometimes (and probably rarely) need to call general (complex) Evaluation. But in those conditions, I do not expect that Evaluation has to change one bit over what it would do if Boxing did not exist. So if I have this right, this is currently a flaw of the current Expression evaluation.

The second complexity inside mathics.core.evaluation is due to the homegrown cache, which is probably somewhat useful until things get rewritten more properly so it is not needed because it is covered by other kinds of caches and because general compound Expression evaluation is not called unnecessarily.

Evaluation is a fundamental or "core" aspect of all objects. For now, I don't see a problem with explicitly specifying what evaluation is for the different kinds of atoms. Except for (compound) Expression evaluation, the code is not so large. And the complexity of (compound) Expression evaluation was alluded to before.

Now we come to mathics.eval. These are intended helping evaluating the builtin functions which lie above the level of the core objects and core concepts. A reason it is good to have these in a separate module is that they can be called outside of the context of a specific single builtin. We saw for example that cross-product kind of combination is useful in several Bulltin-functions. Or in here we have a common setup routine for calling sympy or mpmath used in implementing many builtin functions.

Also, we can avoid having to call the slow general compound Expression evaluation for something like a simple arithmetic expression by calling just the parts that we need to call and skip stuff like ordering arguments, looking for rewrite rules and so on. But note, some discretion and judgement should be used here, because we can get some very ugly and complex code for very marginal gains.

Notice that this customization and reusability aspect has very little use for the evaluation routine for atoms or compound Expressions. It is hard to imagine situations where one wants to call compound Expression rewrite_apply_eval_step() outside of Expression. Or the same thing is true for the other specialized evaluation routines. (There might be, but it feels pathological and probably would be is simpler and clearer to go through normal mechanism).

Having said this, here might be a case for splitting core evaluation into a separate module in mathics.core. But such kinds of things I think we should discuss at a higher level before coding. And please let's not contemplate this until after the next release. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants