Correct LHS evaluation in SetDelayed assignment #603

mmatera · 2022-11-10T02:14:03Z

This PR follows #602. After fixing OneIdentity, it was possible to fix a little bit the behavior of Set*, in a way that in expressions like

F[x_]:=G[x]

H[F[x_]]:= x^2

the assignment is done to H[G[x_]] instead of H[F[x_]], i.e., the argument of the LHS is evaluated before the assignment. Also, more comments on the routines of the mathics.builtin.assignment.internals is provided.

…ay from the conditional.

mmatera · 2022-11-10T02:15:10Z

mathics/core/list.py

+        # decide whether doing this this is warranted.
+        new_list._build_elements_properties()
+        new_list.value = None
+        return new_list


Notice that in Expression.evaluate_elements, a new expression is returned.

Thanks for catching! I guess this was a bug fixed then. If this was causing a problem, I am curious as to where it was causing a problem.

catch not known attributes in ClearAttributes and SetAttributes

`mathics.builtin.attributes`: * apply -> eval * Add WMA links add * Class names ordered alphabetically * Hard breaks in docstring removed * Some incorrect references to "leaves" changed to "attributes" `mathics.core.attributes`: * Add short comments above flag value

…andards Update 'attributes' for current standards

…ay from the conditional.

fix a bug that makes that URLSave always fails

improving clarity in Builtin.contribute

Adding comments and tests for Assignment

symbols.py: system_symbols() -> symbol_set(); "systems_symbols" name is too close Symbol( System` to module systemsymbols. Also, we now require symbols as parameters), not strings. systemsymbols.py: more system symbols

It appears this was originally Pattern.create. I suspect due to bad modularity and a lack of understandig Python that an import could be added inside the routine, this static method got moved outside of the class. Later on, the modularity was fixed, but the hack persisted. These kinds of code smells side effects of poor communication.

wehn it gets fixed.

improve `OneIdentity` builtin

rocky · 2022-11-13T02:26:16Z

@mmatera I believe we are getting conflicts because this brange duplicates commits in other branches and assumed that the other branches weren't going to change before commit.

If this follows #602 I think a better strategy is to base the branch off of #602 and create the PR to go back into #602.

mmatera · 2022-11-13T02:59:52Z

@mmatera I believe we are getting conflicts because this brange duplicates commits in other branches and assumed that the other branches weren't going to change before commit.

If this follows #602 I think a better strategy is to base the branch off of #602 and create the PR to go back into #602.

Yep, to make this work I had to merge the previous version of the oneidentity pr. This needs a rebase now, and yes, that strategy sound reasonable.

rocky · 2022-11-13T08:52:17Z

mathics/builtin/assignments/internals.py

@@ -122,6 +124,32 @@ def is_protected(tag, defin):
    return A_PROTECTED & defin.get_attributes(tag)


+def normalize_lhs(lhs, evaluation):


If seems to me that this function and, more generally, the entire module should be moved to mathics.core.

Let me explain a little here. The code was originally organized around "builtins" which were put in the mathics.core.builtin module. In particular a Builtin class definition contained:

Mathics Pattern matching signature

eval methods

low-level routines to support eval methods

Boxing routines

high-level format and low-level render routines

doctests and some pytests

other miscellaneous things.

I suppose for a toy implementation for a subset of the Mathematica, this might have been fine to start out with.

What I think we are finding is that this is just not scalable or production quality.

Suggested function signature with type annotation:

def normalize_lhs(lhs: Expression, evaluation: Evaluation) -> Tuple(Expression, str):

By the way, although there are lots of little comments, this style of function is a great improvement over the style that we were left with. Thanks!

rocky · 2022-11-13T08:54:12Z

mathics/builtin/assignments/internals.py

+      a shallow conditional expression
+      ( Conditional[Conditional[...],tst] -> Conditional[uncondlhs, tst])
+      with `uncondlhs` the result of strip all the conditions from lhs.
+    * if `uncondlhs` is not a `List` or a `Part` expression, evaluate the


A small note here. I have been using the RsT tagging inside docstrings like this. So for example:

* if ``uncondlhs`` is not a ``List`` or ``Part`` expression ...

The reason I do this that this is what Sphinx uses by default when told to create system-level documentation.

Small thing, as a variable name lhs is fine, when not a variable symbol, I think caps is better: Process the LHS because it is in fact an acronym like IBM.

the name "uncondlhs" feels awkward. Please consider using underscores to separate words, e.g. "uncond_lhs" or the maybe "stripped_lhs".

rocky · 2022-11-13T09:23:17Z

mathics/core/expression.py

@@ -538,6 +538,10 @@ def evaluate(
        return expr

    def evaluate_elements(self, evaluation) -> "Expression":
+        """
+        return a new expression with the same head, and the
+        evaluable elements evaluated.


"evaluable" is jarring. Maybe we can just remove this word?

From an old IBM manual:

Depressing the Power On key initiates the power on sequence.
Depressing the Power Off key initiates the power off sequence.
Depressing the Power On key when the power is already on has no effect.
Depressing the Power Off key when the power is already off has no effect.

Similarly trying to evaluation an element where there is nothing to evaluate has no effect, so suspect does not need to be spelled out.

rocky · 2022-11-13T09:25:55Z

mathics/core/list.py

@@ -86,6 +86,10 @@ def __str__(self) -> str:

    # @timeit
    def evaluate_elements(self, evaluation):


Suggested signature:

def evaluate_elements(self, evaluation: Evaluation) -> Expression:

Yes, I realize this was from before. I am trying to up our game in terms of quality incrementally as we improve things.

rocky · 2022-11-13T09:44:30Z

test/builtin/test_assignment.py

+        (
+            "F[x_]:=G[x]; H[F[y_]]^:=Q[y]; ClearAll[F]; {H[G[5]],H[F[5]]}",
+            "{Q[5], H[F[5]]}",
+            "The arguments on the LHS are evaluated before the assignment",


It would be nice to change the failure message to something different than the others above and below. In particular, how is this test different from the others?

rocky · 2022-11-13T09:47:37Z

@mmatera All very good and much needed. Thanks!

The coding style in this new code has been greatly improved over what has come before in the code base (although if it is not too much I am pushing for a tad more).

Move core-like assignment interals out of builtins and into core. (We may want to split up core more in the future though) Better sort long list of assignment methods alphabetically Some small RstT tagging in a docstring Remove nonexistent word "evaluable" Add yet another type annotation to a signature Make test assert failure messages unique

Changes suggested in PR review

rocky · 2022-11-15T03:51:14Z

LGTM - merge when satisfied

mmatera · 2022-11-15T03:57:27Z

Ok, when the ci finishes, let's merge this and then we can continue with the other parts

mmatera added 10 commits November 7, 2022 10:42

improving clarity in Builtin.contribute

8dbbe6e

adding comments. adding tests

6dd03d5

fix test for SetDelayed. Move special case for Set with LHS a list aw…

ff86f72

…ay from the conditional.

more on modularize assignment. assign_elementary->assign

a514882

fixing set_eval

5c5d85d

adding tests for OneIdentity

11f29ba

fix OneIdentity

88be642

remove comment

0c60ac5

Merge branch 'fix_one_identity' into fix_set_eval

7a62c66

fix pytest

8cd763a

mmatera commented Nov 10, 2022

View reviewed changes

mmatera mentioned this pull request Nov 10, 2022

Fix set ops another round #605

Closed

mmatera and others added 18 commits November 10, 2022 18:24

fix a bug that makes that URLSave always fails

58f808e

fix WriteString standard output

b0b52b5

catch not known attributes in ClearAttributes and SetAttributes

71b082b

Merge pull request #608 from Mathics3/fix_Set_and_Clear_Attributes

8a86511

catch not known attributes in ClearAttributes and SetAttributes

Merge pull request #607 from Mathics3/update-attribute-for-current-st…

3528e0d

…andards Update 'attributes' for current standards

adding comments. adding tests

513bfc5

fix test for SetDelayed. Move special case for Set with LHS a list aw…

e92da0f

…ay from the conditional.

more on modularize assignment. assign_elementary->assign

a728701

Merge pull request #606 from Mathics3/fix_URLSave

9f9c5b8

fix a bug that makes that URLSave always fails

Merge pull request #600 from Mathics3/fix_contribute

d88ab76

improving clarity in Builtin.contribute

Merge pull request #601 from Mathics3/documenting_assignment

ad6b87a

Adding comments and tests for Assignment

fix OneIdentity

92e6b0c

remove comment

cd74aa9

Handle optional with a first element that is not a Pattern[]

cb2ca92

Update examples in OneIdentity

afcf467

More pervasive use of Symbols

454bbaa

symbols.py: system_symbols() -> symbol_set(); "systems_symbols" name is too close Symbol( System` to module systemsymbols. Also, we now require symbols as parameters), not strings. systemsymbols.py: more system symbols

rocky and others added 4 commits November 12, 2022 20:06

Add function signature; straighten import issue

d33ca93

Put test_rules_patterns tests where they belong

318a2b8

Add note to add skipped example as a doctest ...

de5bbbe

wehn it gets fixed.

Merge pull request #602 from Mathics3/fix_one_identity

cdd8e6a

improve `OneIdentity` builtin

merge

b829b09

rocky reviewed Nov 13, 2022

View reviewed changes

rocky changed the title ~~Fix set eval~~ Correct LHS evaluation in Set assignment Nov 13, 2022

rocky changed the title ~~Correct LHS evaluation in Set assignment~~ Correct LHS evaluation in Set, SetDelayed assignment Nov 13, 2022

rocky changed the title ~~Correct LHS evaluation in Set, SetDelayed assignment~~ Correct LHS evaluation in SetDelayed assignment Nov 13, 2022

rocky and others added 3 commits November 14, 2022 22:18

Move ASSIGNMENT_FUNCTION_MAP into core

ca35d60

Merge pull request #622 from Mathics3/improve-SetDelayed-LHS-assignment

73a369b

Changes suggested in PR review

mmatera merged commit 2ffdf02 into master Nov 15, 2022

mmatera deleted the fix_set_eval branch November 15, 2022 04:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct LHS evaluation in SetDelayed assignment #603

Correct LHS evaluation in SetDelayed assignment #603

mmatera commented Nov 10, 2022 •

edited by gitpod-io bot

Loading

mmatera Nov 10, 2022

rocky Nov 13, 2022 •

edited

Loading

rocky commented Nov 13, 2022

mmatera commented Nov 13, 2022

rocky Nov 13, 2022 •

edited

Loading

rocky Nov 13, 2022

rocky Nov 13, 2022

rocky Nov 13, 2022 •

edited

Loading

rocky Nov 13, 2022

rocky Nov 13, 2022

rocky Nov 13, 2022 •

edited

Loading

rocky Nov 13, 2022

rocky Nov 13, 2022

rocky commented Nov 13, 2022 •

edited

Loading

rocky commented Nov 15, 2022 •

edited

Loading

mmatera commented Nov 15, 2022 •

edited

Loading

		@@ -122,6 +124,32 @@ def is_protected(tag, defin):
		return A_PROTECTED & defin.get_attributes(tag)


		def normalize_lhs(lhs, evaluation):

		@@ -86,6 +86,10 @@ def __str__(self) -> str:

		# @timeit
		def evaluate_elements(self, evaluation):

Correct LHS evaluation in SetDelayed assignment #603

Correct LHS evaluation in SetDelayed assignment #603

Conversation

mmatera commented Nov 10, 2022 • edited by gitpod-io bot Loading

mmatera Nov 10, 2022

Choose a reason for hiding this comment

rocky Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

rocky commented Nov 13, 2022

mmatera commented Nov 13, 2022

rocky Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

rocky Nov 13, 2022

Choose a reason for hiding this comment

rocky Nov 13, 2022

Choose a reason for hiding this comment

rocky Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

rocky Nov 13, 2022

Choose a reason for hiding this comment

rocky Nov 13, 2022

Choose a reason for hiding this comment

rocky Nov 13, 2022 • edited Loading

Choose a reason for hiding this comment

rocky Nov 13, 2022

Choose a reason for hiding this comment

rocky Nov 13, 2022

Choose a reason for hiding this comment

rocky commented Nov 13, 2022 • edited Loading

rocky commented Nov 15, 2022 • edited Loading

mmatera commented Nov 15, 2022 • edited Loading

mmatera commented Nov 10, 2022 •

edited by gitpod-io bot

Loading

rocky Nov 13, 2022 •

edited

Loading

rocky Nov 13, 2022 •

edited

Loading

rocky Nov 13, 2022 •

edited

Loading

rocky Nov 13, 2022 •

edited

Loading

rocky commented Nov 13, 2022 •

edited

Loading

rocky commented Nov 15, 2022 •

edited

Loading

mmatera commented Nov 15, 2022 •

edited

Loading