Merged
Conversation
Of particular interest were the changes to rewrite_apply_step which is where we will be focusing.
rocky
force-pushed
the
leaves-element-rename-part2
branch
from
6e34c0e to
6872983
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Also there are some S-Expression to M-expression changes and things of that kind.
But of particular interest were the changes of "leaf" to "element" in to rewrite_apply_step which is where we will be focusing.
@mmatera @TiagoCavalcante I generally think of myself as a live and let live kind of person - if there is a mess in the code and I don't have to be confronted with it I am happy to ignore it.
The problem is this leaves versus elements confusion is right at the place I am looking at now in rewrite_apply_eval_step.
So then I ask myself do I want to just keep reminding myself, "black" really means "red "and "up" really means "left" or do we fix this so I don't have to do this and get confused.
BEGIN RANT
I think what annoys me most is that every time I want go off and look at something, there is already a mess that has been left there from the beginning that we need to deal with before we can even consider getting to the part I might be interested in. And so anything I am interested in has generally put off indefinitely (so far).
And the wrongness has been so pervasive and in such an advanced state that we can't even fix it in one go. Redoing documentation is like that. Getting to Jupyter is like that too.
END RANT
So here we are at another PR.
There are tons of other conversions from leaf to element that probably should occur. In a sense I am not happy that we are in this hybrid state that sometimes we have to still refer to an element as a "leaf" or "leaves" when it is not.
But on the other hand I'd rather be in a hybrid state pointing to the right direction rather than always wrong, and being complacent about it being wrong.
BTW - Here is what I was doing that motivated this: I read in the Mathematica book for version 5 that describes the rewrite_apply_eval step that the first thing to do is to check to see if the element (or stupidly called "leaf" in that section of our code) is a literal value, it can just return the value and not have to do any fancy rewrite/apply.
It doesn't look like we do that currently. @mmatera do you think this simple check is still correct? I suspect it might result in a noticeable speed improvement if it is still accurate.