Skip to content

Fix set 3rd round #619

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.

Sign up for GitHub

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

Closed
mmatera wants to merge 10 commits into from
Closed

Fix set 3rd round #619

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f4eb4ce
Add SetAttributes length check and ...
rocky Nov 13, 2022
069deea
Merge pull request #615 from Mathics3/SetAttribute-arg-check
rocky Nov 13, 2022
c1b5fa3
fix get_sort_key second argument and BoxExpression.sameQ. With this, …
mmatera Nov 13, 2022
475cb9d
adding test
mmatera Nov 13, 2022
1f87088
simplifying assignment. Now all the tests pass.
mmatera Nov 14, 2022
784e723
simplify and improve assign internals
mmatera Nov 14, 2022
c216a93
comment on process_rhs_conditions
mmatera Nov 14, 2022
2ffdf02
Correct LHS evaluation in SetDelayed assignment (#603)
mmatera Nov 15, 2022
ed3b5bb
merge
mmatera Nov 15, 2022
d69e338
merge
mmatera Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions mathics/builtin/assignments/internals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,27 @@ def process_assign_messagename(self, lhs, rhs, evaluation, tags, upset):
def process_rhs_conditions(lhs, rhs, evaluation, condition=None):
"""
lhs = Condition[rhs, test] -> Condition[lhs, test] = rhs

This function is necesary as a temporal patch for a bug
in the pattern matching / replacement code.

In WMA,

``f[4]/.(f[x_]:> (p[x]/;x>0))``

shold return

``p[4]``

while ``f[-4]/.(f[x_]:> (p[x]/;x>0))``
would result in ``f[-4]``

In Mathics, the first case results in
``p[4]/; 4>0``
while the second,
``p[-4]/; -4>0``

This should be handled in the apply method of the BaseRule class.
Copy link
Copy Markdown
Member

@rocky rocky Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This was going to be my point as well. Too often it feels that when we try to run to run some WL code and that fails, the immediate solution is well, let's just add a bunch of flags and tests on symbols in Python code address the specific case we see.

This is done instead of seeing the problem as a manifestation of some larger structural problem, or feature lacking in the way particular built-in functions should work.

I would prefer to see fixing the root cause issues in favor of the workaround coding.

(You probably mean eval method, but okay.)

Copy link
Copy Markdown
Contributor Author

@mmatera mmatera Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This was going to be my point as well. Too often it feels that when we try to run to run some WL code and that fails, the immediate solution is well, let's just add a bunch of flags and tests on symbols in Python code address the specific case we see.

No, the idea is to identify these issues, and then try to understand how it works and how it should work, and then fix it.

This is done instead of seeing the problem as a manifestation of some larger structural problem, or feature lacking in the way particular built-in functions should work.

A manifestation of a large structural problem is the performance issue, and the fact that we can not load wl modules. All of this is about to narrow the different issues and try to determine what should be the pattern.

I would prefer to see fixing the root cause issues in favor of the workaround coding.

The workaround was there for a while. What I did here is to determine why this workaround was needed to reproduce the WMA behavior. As the picture gets clear, we can continue removing these issues, like the one with oneidentity. What I am trying to avoid is to pretend to fix many things in the same PR.

(You probably mean eval method, but okay.)

I said apply, because is the current name of the method. Then, if you think that is more clear/ accurate to say "evaluate a replacement rule over an expression" over "apply a replacement rule over an expression" let's change the name of the method.

Copy link
Copy Markdown
Member

@rocky rocky Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The workaround was there for a while. What I did here is to determine why this workaround was needed to reproduce the WMA behavior. As the picture gets clear, we can continue removing these issues, like the one with oneidentity. What I am trying to avoid is to pretend to fix many things in the same PR.

So what I am thinking is let's focus on removing the workarounds that we can fix rather than changing workarounds.

I know of some on the horizon but that could be fixed now, like #597 and the box precedence operator. Stuff like this keeps rolling in though which from my standpoint isn't helping as much as it is delaying getting more root issues fixed.

Copy link
Copy Markdown
Member

@rocky rocky Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I said apply, because is the current name of the method. Then, if you think that is more clear/ accurate to say "evaluate a replacement rule over an expression" over "apply a replacement rule over an expression" let's change the name of the method.

I just looked this up in the code. You are correct - this method named apply is doing application, not evaluation. And this reinforces why it would be better to have terminology cleaned up - I often do not know if a term is used in the right sense or the wrong sense and conceptually they mean different things so I get more confused.

"""
# To Handle `OptionValue` in `Condition`
rulopc = build_rulopc(lhs.get_head())
Expand Down