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

List as atom #121

Closed
wants to merge 2 commits into from
Closed

List as atom #121

wants to merge 2 commits into from

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Jan 20, 2022

In this PR, a basic proposal about how to handle List evaluations is implemented. Same ideas could be implemented to other frequently used symbols, or certain direct sympy expressions.

@TiagoCavalcante
Copy link
Contributor

TiagoCavalcante commented Jan 20, 2022

ExpressionList is something that would benefit from ExpandOnce.

@@ -611,6 +611,8 @@ def get_string_value(self):

def sameQ(self, expr) -> bool:
"""Mathics SameQ"""
if isinstance(expr, BoxConstruct):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a general method like SameQ is asking about a specific kind of thing like BoxConstruct that should be a harbinger that there is a design problem and that a general concept is missing.

At a minimum, a lot of explaining is needed as to why sameQ which is testing whether something is identical needs to understand why something is a Boxing construct.

Sometimes instead there is a fundamental concept that needs to be made explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because if BoxConstruct compares against other kind of expression, it can delegate the implementation to the other object. But if expr is also a BoxConstruct, this would produce an infinite loop. However, in this case, we just need to compare if self and expr are the same Python object.

@mmatera
Copy link
Contributor Author

mmatera commented Jan 20, 2022

ExpressionList is something that would benefit from ExpandOnce.

Actually, the idea of this experiment is to see if we can avoid the need to tag leaves in an expression as ExpandOnce. When the evaluator finds an expression of the form Expression(SymbolList, ...), the expression is replaced by something with a an evaluation logic that depends on the kind of leaves. If all the leaves are numbers, (or maybe, undefined symbols) no more further evaluations will be carried on. Otherwise, at least we avoid looking at definitions and attributes for Symbol`List.

@rocky
Copy link
Member

rocky commented Jan 20, 2022

ExpressionList is something that would benefit from ExpandOnce.

Actually, the idea of this experiment is to see if we can avoid the need to tag leaves in an expression as ExpandOnce. When the evaluator finds an expression of the form Expression(SymbolList, ...), the expression is replaced by something with a an evaluation logic that depends on the kind of leaves. If all the leaves are numbers, (or maybe, undefined symbols) no more further evaluations will be carried on. Otherwise, at least we avoid looking at definitions and attributes for Symbol`List.

Personally, I'd like to see a bigger picture description of the overall evaluation process and in that it would be clearer to discuss under what conditions we need pattern matching or reevaluation or not.

In that it is also becoming clear to me that we should have means for alternate representations of expressions. In some cases for example there could be a SymPy object that is equivalent. Possibly others like numpy, mpmath, scipy, compiled form - I just don't know the details of these well.

I do know however that this kind of incremental change might not be the best approach. It hasn't served us that well so far.

@mmatera
Copy link
Contributor Author

mmatera commented Jan 20, 2022

ExpressionList is something that would benefit from ExpandOnce.

Actually, the idea of this experiment is to see if we can avoid the need to tag leaves in an expression as ExpandOnce. When the evaluator finds an expression of the form Expression(SymbolList, ...), the expression is replaced by something with a an evaluation logic that depends on the kind of leaves. If all the leaves are numbers, (or maybe, undefined symbols) no more further evaluations will be carried on. Otherwise, at least we avoid looking at definitions and attributes for Symbol`List.

Personally, I'd like to see a bigger picture description of the overall evaluation process and in that it would be clearer to discuss under what conditions we need pattern matching or reevaluation or not.

I think that, at this point, I would be in a position to describe the (current) evaluation algorithm with a certain detail. If you think it would be useful, I can try to write a draft describing the steps, or just by adding comments to the code in a new branch.

In that it is also becoming clear to me that we should have means for alternate representations of expressions. In some cases for example there could be a SymPy object that is equivalent. Possibly others like numpy, mpmath, scipy, compiled form - I just don't know the details of these well.

I do know however that this kind of incremental change might not be the best approach. It hasn't served us that well so far.

Incremental changes are not the solution but can provide some idea of what we need to change, and what would be the side effects.

@rocky
Copy link
Member

rocky commented Jan 20, 2022

In that it is also becoming clear to me that we should have means for alternate representations of expressions. In some cases for example there could be a SymPy object that is equivalent. Possibly others like numpy, mpmath, scipy, compiled form - I just don't know the details of these well.
I do know however that this kind of incremental change might not be the best approach. It hasn't served us that well so far.

Incremental changes are not the solution but can provide some idea of what we need to change, and what would be the side effects.

Sure, in many cases this kind of thing can work. Here, in this specific case can I ask please can we have a moratorium on the little tweaks and suggested changes here and there for purposes of performance?

I'd like some quiet time to take a bigger more holistic view of what's going on and all of this is noise and distraction. Others have had time and free reign to try things out. It hasn't been working out.

In taking a more holistic view, in some cases things may even get slower micro-improvement wise. But that is okay if in we wind up in having something that is more modular, more understandable, more scalable, and will most likely be — given how far off we are from how fast this should run — much faster too.

@rocky
Copy link
Member

rocky commented Jan 20, 2022

I think that, at this point, I would be in a position to describe the (current) evaluation algorithm with a certain detail. If you think it would be useful, I can try to write a draft describing the steps, or just by adding comments to the code in a new branch.

@mmatera yes, it would be very helpful to describe in as high a level as we can, the evaluation process. We started this in Code Overview

If that needs revising or expanding, please do that.

@mmatera
Copy link
Contributor Author

mmatera commented Jan 20, 2022

ExpressionList is something that would benefit from ExpandOnce.

Actually, the idea of this experiment is to see if we can avoid the need to tag leaves in an expression as ExpandOnce. When the evaluator finds an expression of the form Expression(SymbolList, ...), the expression is replaced by something with a an evaluation logic that depends on the kind of leaves. If all the leaves are numbers, (or maybe, undefined symbols) no more further evaluations will be carried on. Otherwise, at least we avoid looking at definitions and attributes for Symbol`List.

Personally, I'd like to see a bigger picture description of the overall evaluation process and in that it would be clearer to discuss under what conditions we need pattern matching or reevaluation or not.

I think that, at this point, I would be in a position to describe the (current) evaluation algorithm with a certain detail. If you think it would be useful, I can try to write a draft describing the steps, or just by adding comments to the code in a new branch.

In that it is also becoming clear to me that we should have means for alternate representations of expressions. In some cases for example there could be a SymPy object that is equivalent. Possibly others like numpy, mpmath, scipy, compiled form - I just don't know the details of these well.
I do know however that this kind of incremental change might not be the best approach. It hasn't served us that well so far.

Incremental changes are not the solution but can provide some idea of what we need to change, and what would be the side effects.

Sure, in many cases this kind of thing can work. Here, in this specific case can I ask please can we have a moratorium on the little tweaks and suggested changes here and there for purposes of performance?

I'd like some quiet time to take a bigger more holistic view of what's going on and all of this is noise and distraction. Others have had time and free reign to try things out. It hasn't been working out.

OK, so @rocky, a few questions:

  1. Should I close this PR? The only reason to keep this open is just to collect/discuss ideas.
  2. Would be useful to have a PR with more comments on the part of the code related to the evaluate method?
  3. Apart to do not introducing more tweaks, is there anything else we can do for helping you with the redesign?

In taking a more holistic view, in some cases things may even get slower micro-improvement wise. But that is okay if in we wind up in having something that is more modular, more understandable, more scalable, and will most likely be — given how far off we are from how fast this should run — much faster too.

@rocky
Copy link
Member

rocky commented Jan 20, 2022

  1. Should I close this PR? The only reason to keep this open is just to collect/discuss ideas.

The ideas and motivation behind this are good. So we should forget the general idea. The specific code I don't think it likely we will modify or incrementally change this code. So in that sense, yes, I guess close it.

  1. Would be useful to have a PR with more comments on the part of the code related to the evaluate method?

It would be good to start annotating the existing code. If you have comments, concerns, or thoughts about how to change things, then yes, that is great.

  1. Apart to do not introducing more tweaks, is there anything else we can do for helping you with the redesign?

I expect this to be a group process, with everyone involved all the way along. But first I think we all need to take stock of what we have, how this works, how it might work and so on.

So right now I would say focus on elaborating and explaining what's there.

The other aspect that I think we are extremely week on is in testing little parts. for example the simple Lisp/WL Symbol should be testable independent of the rest of the code. The tests might include when we try to create two Symbols, they map to the same thing. And that we can extract a string back from the Symbol.

When I look at https://reference.wolfram.com/language/ref/Symbol.html there are basic things described their, like Unique and Context that would seem to help support whether Symbol has the right properties, but we don't seem to have those.

The same things would go for pattern matching.

I don't understand precisely Atom, Instanceable the current definition of Mathics Symbol and the differences between these and Builtin and how that corresponds to published parts of Mathematica.

In taking a more holistic view, in some cases things may even get slower micro-improvement wise. But that is okay if in we wind up in having something that is more modular, more understandable, more scalable, and will most likely be — given how far off we are from how fast this should run — much faster too.

@mmatera
Copy link
Contributor Author

mmatera commented Jan 20, 2022

The ideas and motivation behind this are good. So we should forget the general idea. The specific code I don't think it likely we will modify or incrementally change this code. So in that sense, yes, I guess close it.

OK

  1. Would be useful to have a PR with more comments on the part of the code related to the evaluate method?

It would be good to start annotating the existing code. If you have comments, concerns, or thoughts about how to change things, then yes, that is great.

I am doing this a little. I will create a new branch with just the docstrings / comments.

  1. Apart to do not introducing more tweaks, is there anything else we can do for helping you with the redesign?

I expect this to be a group process, with everyone involved all the way along. But first I think we all need to take stock of what we have, how this works, how it might work and so on.

So right now I would say focus on elaborating and explaining what's there.

OK, then let's go in that direction.

@mmatera mmatera closed this Jan 20, 2022
@mmatera
Copy link
Contributor Author

mmatera commented Jan 20, 2022

@rocky this is the branch
commented-evaluation-process

@rocky
Copy link
Member

rocky commented Jan 21, 2022

Ok, but I think this misses the point then or is looking at things in a too narrow way that is missing the bigger picture which I'll try to describe elsewhere. The notions from programming languages like literal values, unbound and bound expressions are the higher level concepts we should consider.

I agree though that it probably would be good to allow for alternate kinds of representations of "Expression".

Please hold off on the suggestions. I know it doesn't feel like helping by holding off, but, really, it helps.

@rocky
Copy link
Member

rocky commented Jan 21, 2022

@rocky this is the branch commented-evaluation-process

@mmatera My apologies of repeating more or less what I wrote before. I didn't understand that this comment was referring to another branch. I mistakenly thought "this branch" meant this closed PR branch. I've updated this to add the link and I'll look at this branch over the weekend.

Again, my apologies.

@rocky rocky deleted the list_as_atom branch July 11, 2022 07:02
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

3 participants