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

Fix the bug in evaluation of expressions with elements of the form Unevaluated[elem] #628

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

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Nov 19, 2022

This PR is a proposal to fix #122. The idea is to temporarily replace elements of the form Expression(SymbolUnevaluated, elem) by an instance of a wrapped object UnevaluatedWrapper(elem).
A larger and improved set of pytest was incorporated to test_evaluation.

improving tests for expressions containg elements of the form Unevaluated[...]
@rocky
Copy link
Member

rocky commented Nov 21, 2022

This PR is a proposal to fix #122. The idea is to temporarily replace elements of the form Expression(SymbolUnevaluated, elem) by an instance of a wrapped object UnevaluatedWrapper(elem). A larger and improved set of pytest was incorporated to test_evaluation.

I haven't looked at this in detail, but from this high-level description I don't think this is going in the right direction.

Instead of inventing solutions, I think we should be following common practice of separating the idea of a symbol (and unbound symbol) versus a bound symbol which is a symbol along with a binding to a value.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 21, 2022

This PR is a proposal to fix #122. The idea is to temporarily replace elements of the form Expression(SymbolUnevaluated, elem) by an instance of a wrapped object UnevaluatedWrapper(elem). A larger and improved set of pytest was incorporated to test_evaluation.

I haven't looked at this in detail, but from this high-level description I don't think this is going in the right direction.

Instead of inventing solutions, I think we should be following common practice of separating the idea of a symbol (and unbound symbol) versus a bound symbol which is a symbol along with a binding to a value.

I do not see how it fits with the evaluation loop described in WR, but OK. In any case, what I tried to do here is to look for a solution that is localized in the routine that must be fixed, and again, to provide a fix to the behavior, and a set of tests that must be passed for a better, clear and canonical proposal of fixing.

@rocky
Copy link
Member

rocky commented Nov 21, 2022

This PR is a proposal to fix #122. The idea is to temporarily replace elements of the form Expression(SymbolUnevaluated, elem) by an instance of a wrapped object UnevaluatedWrapper(elem). A larger and improved set of pytest was incorporated to test_evaluation.

I haven't looked at this in detail, but from this high-level description I don't think this is going in the right direction.
Instead of inventing solutions, I think we should be following common practice of separating the idea of a symbol (and unbound symbol) versus a bound symbol which is a symbol along with a binding to a value.

I do not see how it fits with the evaluation loop described in WR, but OK. In any case, what I tried to do here is to look for a solution that is localized in the routine that must be fixed, and again, to provide a fix to the behavior, and a set of tests that must be passed for a better, clear and canonical proposal of fixing.

It is about how variables get values. The evaluation loop gets values from variables/symbols.

Yes, I get that the way this is working is from specific bug to specific hand-crafted solution to that bug. I tried with my Rubik's cube analogy to describe how that sometimes can lead one to astray.

Here is another: in sailing there is "line of sight navigation" where you find a landmark sail to it and then find another landmark. If all you need to do is move a short distance, this can be done okay. The polynesians worked that way for their local islands. However if you want to move great distances, you need to take a more global view.

Or if you want this explained in theory: going to the local maxima of a curve doesn't always find the maximum.

@rocky rocky mentioned this pull request Nov 21, 2022
@@ -27,7 +27,12 @@
)
from mathics.core.convert.python import from_python
from mathics.core.convert.sympy import SympyExpression, sympy_symbol_prefix
from mathics.core.element import BaseElement, ElementsProperties, EvalMixin, ensure_context
from mathics.core.element import (
Copy link
Member

Choose a reason for hiding this comment

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

Are we having isort/black wars? isort should be followed by black. isort will respect black's formatting of isort'ed code. However running black and then isort, may cause isort to change the formatting when isort wants to reorder things.

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.

Unevaluated
2 participants