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 pat->Condition[...] apply rules #621

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Nov 15, 2022

This PR fixed the bug described in #619 regarding the result of applying this kind of rule. The problem was not in the apply method itself but in the do_replace method.


def test_rule_repl_cond():
for str_expr, str_expected, message in (
("f[x]/.(f[u_]->u^2/; u>3/; u>2)", "f[x]", "conditions are not evaluated"),
Copy link
Member

Choose a reason for hiding this comment

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

I just tried this on galatea and I don't see these results. Here is what I see:

>> f[x]/.(f[u_]->u^2/; u>3/; u>2)
 = x ^ 2 /; x > 3 /; x > 2
>> f[1.]/.(f[u_]->u^2/; u>3/; u>2)
 = 1. /; 1. > 3 /; 1. > 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. When I tested it, I did it with delayed rules instead. There is something that I still am missing.

super(Rule, self).__init__(pattern, system=system)
self.replace = replace
self.delayed = delayed
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 property helps to distinguish if the Rule comes from a Rule expression or a RuleDelayed expression, and it is needed to handle the different behavior observed in WMA. I do not see a reason for this different behavior, but maybe it is, and maybe there is a different behavior in other contexts.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 15, 2022

Still, there is something that is not covered with this: consider the following code:

F[k_] := 1 /; (k == 1);

F[k_] := 2  /; (k > 1);

F[k_] := A[k] ;

F[k_] := B[k] ;

DownValues[F]

In Mathics, the result is

{HoldPattern[F[k_]] ⧴ B[k]}

because the four lines sets over the same LHS pattern. On the other hand, the same code in WMA results in

{HoldPattern[F[k_]] :> 1 /; k == 1, HoldPattern[F[k_]] :> 2 /; k > 1, HoldPattern[F[k_]] :> B[k]}

so rules with the same LHS but different RHS is also overwritten, except if the RHS is a Condition expression.

@rocky
Copy link
Member

rocky commented Nov 15, 2022

Still, there is something that is not covered with this: consider the following code:

F[k_] := 1 /; (k == 1);

F[k_] := 2  /; (k > 1);

F[k_] := A[k] ;

F[k_] := B[k] ;

DownValues[F]

In Mathics, the result is

{HoldPattern[F[k_]] ⧴ B[k]}

because the four lines sets over the same LHS pattern. On the other hand, the same code in WMA results in

{HoldPattern[F[k_]] :> 1 /; k == 1, HoldPattern[F[k_]] :> 2 /; k > 1, HoldPattern[F[k_]] :> B[k]}

so rules with the same LHS but different RHS is also overwritten, except if the RHS is a Condition expression.

Maybe ask on https://mathematica.stackexchange.com/ what is up here and more generally how to think about how WL groups assignment categories?

@mmatera
Copy link
Contributor Author

mmatera commented Nov 15, 2022

Actually, I am looking there, but at existing related questions.

@mmatera mmatera force-pushed the fix_issue_in_rules_with_conditional_replacement branch from 2d37839 to ed98034 Compare November 15, 2022 15:58
@mmatera mmatera force-pushed the fix_issue_in_rules_with_conditional_replacement branch from ed98034 to 62cf433 Compare November 16, 2022 12:53
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.

2 participants