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

add an "ifCondition" parameter to rules, so you can programatically choose whether a rule applies or not #2901

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ziktar
Copy link

@ziktar ziktar commented Feb 19, 2023

I found that when calling simplify with equations in different orders (such as "x + x^2" vs "x^2 + x"), I'd get different results. I wanted to make a consistent way to sort terms, and found there wasn't a conditional way to apply simplification rules; you can only either make a rule that always applies or make a change based on the entire node tree (which means I'd need to write my own matching code).

As such, I propose adding an optional function, I called "ifCondition" , that you can place on a rule. It gets passed the matches that are being currently looked at, and if the function returns false, then the swap is not made, and if it returns true, then it is made.

@josdejong
Copy link
Owner

Thanks for your PR @ziktar

Two thoughts:

  1. Instead of extending the API of simplify rule with a new option ifCondition, would it be possible to use a function as rule in your case? That is more flexible in general, but it may be more verbose.
  2. The reason that you need this condition is because the expression is not normalized and can have two orders. I remember we had some discussion about improving the normalization in this regard, maybe we should look into improvements in that direction. Maybe it is also possible to check whether the rule is about a commutative operator (like +), then it could try to match if the rule matches in opposite order. Do you have any thoughts on that @gwhitney?

@ziktar
Copy link
Author

ziktar commented Feb 20, 2023

@josdejong

Regarding option 1, one goal I had was to keep the existing powerful node matching that exists in the current rules, so I wouldn't want to use the existing provide-a-single-function rule option that exists today. An improvement to what I did here could be to instead of adding the ifCondition, we could make the r parameter optionally be a function. This function would be called with the matches, and could return false (thus not making a change, which is what ifCondition allows), or it could return a node string response (in the same format that a string r would have), and then that replacement could be used. This would be a little bit trickier to implement (from what I can tell, we just need to make sure that the rule function knows about the expanded forms), but I think it would be possible/better than what I have here.

The end goal for me is to be able to sort terms/factors consistently (such as by polynomial degree in the easy case of a bunch of polynomials and alphabetic for polynomials of the same degree, and using some pattern in the more complicated expressions)

As such, regarding option 2, I imagine that I could get to normalized ordering by checking if the rules matched both current and opposite order, but it would still need some logic to be able to compare the two terms beyond just being c vs v vs ve. So we'd need to be able to use a method that compares two terms, so we'd have to extend the simplify rules logic to be able to specify that function being used to compare them. If we're not letting the user add whatever logic they want, then we'll need to make sure that whatever we expose here is comprehensive, so we'll need to make sure our comparison function can compare any expression; math.compareNatural is the best contender for this, but it currently cannot compare complicated expressions (math.compareNatural(math.parse('x^2'), math.parse('sin(x)')) works but math.compareNatural(math.parse('x^2'), math.parse('x*sin(x)')) does not).

@josdejong
Copy link
Owner

About point 2

It's indeed not trivial. It requires thorough thinking through, maybe best to park these ideas for now and focus on making the simplify rule system more flexible.

About point 1

Just to be clear: I'm not ruling out your initial solution with ifCondition, but exploring if this is indeed the best solution we can come up with.

Interesting idea to make the r dynamic by having it as a callback function. I'm not sure how complex that would be to implement. It may be that the concept would be a bit confusing to understand too.

I was also thinking into a different direction: right now a rule can basically defined high level as { l, r }, or as a low level call back function. But now we want something in between. Would it be possible to extract the logic behind { l, r } into utility functions that you can then use inside your own callback function? Something like matchesRule(node) and applyRule(node, rule)? Or maybe the latter is already enough, so you can write a custom rule like:

function customRule(node) {
  if (satisfiesCondition(node)) {
    return applyRule(node, rule, context)
  }
}

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