More simplify wildcards

[thatcomputerguy0101]

This is a fix to #1406 based on some of the discussion in #1435 that maintains compatibility with the current implementation. It adds several new wildcards for simplification rules according to the list below.

• n - Any node [existed prior to change]
• c - A constant literal (5 or 3.2) [existed prior to change]
• cl - A constant literal; same as c
• ci - A decimal literal (5 or -3.2)
• ce - A constant expression (-5 or √3)
• v - A variable; anything not matched by c (-5 or x) [existed prior to change]
• vl - A variable literal (x or y)
• vi - A non-decimal expression; anything not matched by ci (x or √3)
• ve - A variable expression; anything not matched by ce (x or 2x)

Currently, this PR contains library modifications and basic unit tests for the new code.

[josdejong]

Your PR looks really simple and elegant @thatcomputerguy0101, thanks! Nice job.

I think the documentation of the different wildcards you've put on top should also be in the comments in the code of simplify.js.

Do you have some ideas to implement a couple of rules that actually use the new wildcards? To prove its value 😄 . And does the current unit tests show that the original issue #1406 is fixed now?

[thatcomputerguy0101]

I have fixed the styling preference that you mentioned for the import.

When working on unit tests, I found that commit 556f0ffb5 (a merge from develop) seems to have severely broken the library from my perspective. I tried removing the local branch and then redownloading it, but I got the same result. I have attached the output from the unit tests since I can't find what is causing the errors. I also created an rtf version that still has the original formatting, but GitHub doesn't seem to like those. Math.js Unit Tests.txt

[josdejong]

That's odd. Can it be that something went wrong when merging or so?

The change to [email protected] was quite a big one but should not conflict with your changes. The other changes in v7.0.2 and v7.1.0 should not have any influence on the work in simplify.

What I myself do most of the time when something went wrong with my git is check a new develop branch out in an other folder, and manually put the changes I made (in this case in simplify) again and create a new commit for it. Is often faster than trying to get a messed up git branch fixed again.

[josdejong]

Stupid question: did you run npm install to update to the latest version of the dependencies ([email protected])?

[thatcomputerguy0101]

Well, the stupid question was the right one. Since most changes don't result in different npm modules, I didn't think to do that.

[josdejong]

:+1: great to hear that was the cause.

[josdejong]

I see you did write documentation @thatcomputerguy0101 👍 . Can you please move the docs to the top comment, around line 110 of the file? That comment will be used by the doc generator.

[josdejong]

Thanks for the update. I think the current implementation is solid and good to go.

What do you think about the following comment I made?

Do you have some ideas to implement a couple of rules that actually use the new wildcards? To prove its value 😄 . And does the current unit tests show that the original issue #1406 is fixed now?

[thatcomputerguy0101]

Do you have some ideas to implement a couple of rules that actually use the new wildcards? To prove its value 😄 . And does the current unit tests show that the original issue #1406 is fixed now?

I have added tests based off of #1406. I'm not against adding rules for the other wildcards, but I haven't thought of anything that works in the official ruleset yet.

[josdejong]

I'm not against adding rules for the other wildcards, but I haven't thought of anything that works in the official ruleset yet.

It feels a bit odd to implement a new set of wildcards if we have no idea about an actual use case. Shouldn't we at least go through the existing rules to see where we can improve them, for example by replacing c with the new cd where applicable? I think that was the original issue of #1406 right?

[thatcomputerguy0101]

I have went through the existing rules replacing c with cl or cd (depending on context) and v with vd where applicable. Due to some of the other rules, these changes seem to have no effect on the final "simplified form," but if one were to remove those rules, I expect the resulting behavior to be much more logical now.

[josdejong]

Thanks for getting back on this PR @thatcomputerguy0101 .

I see you indeed replaced occurrences of c with cl (just the same), and repaced some v with vd and c with cd. So, just wondering: for example cd also matches on a negative values like -3.2. Is there a unit test now that captures that this rule now works "better" than the old c and captures new cases? (i.e. a unit test that fails when using the old c?)

[thatcomputerguy0101]

Yes, actually, for the rule vd * (vd * n1 + n2), it used to incorrectly simplify -2 * (-2 * x + y) to 4 * x - 2 * y. Now it preserves the original factored form (slightly reordered to -2 * (y - 2 * x)), similar to how the same expression with positive coefficients is handled. I expect the other variations of that rule to behave similarly. -2 * x + -2 is simplified the same, but is now handled by the n*cd + cd rule instead of the n*vd + vd rule (observed using consoleDebug). For the rule ce + ve -> ve + ce, the modified version successfully reorders -2 - x to -x - 2. The change to vd*cd appears to have no effect with the default ruleset since negatives are already factored out by -n * n1, but it would probably work better with modified rulesets.

Based on that, I've added tests for the two scenarios that have different outputs, but left the other two scenarios untested.

[josdejong]

Thanks for the update @thatcomputerguy0101 , the new unit tests are helpful!

So I think this PR is ready to be merged (one minor open comment only). I'll have another brief look at it coming Monday and then merge it.

[josdejong]

Published now in v11.4.0, thanks again!