fix(rules): make commutative swap choose the closest sibling

 - previously it would swap the siblings blindly and you'd end up with these unnatural "Tree swinging" actions that would move whole chunks of the tree around.
  - when ordering terms we don't move five things around at a time, we move one thing.
  - add test cases to ensure that the commutative property can be used to reorder groups of terms
  - better error logging for test failures

Author: justindujardin

libraries/mathy_python/mathy/rules/commutative_property.py

@@ -89,13 +89,11 @@ def apply_to(self, node: MathExpression):
             node.set_right(a)
             node.set_left(b)
         else:
-            # If it's another add, just swap their
+            # If it's another add, swap their
             # children directly to avoid inner-nesting.
-            one = a.left
             two = a.right
             three = node.right
-            a.set_left(three)
-            a.set_right(one)
+            a.set_right(three)
             node.set_right(two)
 
         node.set_changed()

libraries/mathy_python/mathy/testing.py

@@ -74,11 +74,12 @@ def run_rule_tests(name, rule_class, callback=None):
         before = expression.clone()
         print(ex)
         if "target" in ex:
+            target = ex["target"]
             nodes = rule.find_nodes(expression)
             targets = [n.raw for n in nodes]
-            node = [n for n in nodes if n.raw == ex["target"]]
+            node = [n for n in nodes if n.raw == target]
             targets = "\n".join(targets)
-            assert len(node) > 0, f"could not find target node. targets are:\n{targets}"
+            assert len(node) > 0, f"could not find target: {target}. targets are:\n{targets}"
             node = node[0]
         else:
             node = rule.find_node(expression)

libraries/mathy_python/tests/rules/associative_property.json

@@ -1,12 +1,17 @@
 {
   "valid": [
+    {
+      "input": "4x^3 + g + 5y + 7p^4 + x^3 + 12p^4",
+      "output": "4x^3 + g + 5y + 7p^4 + (x^3 + 12p^4)",
+      "target": "4x^3 + g + 5y + 7p^4 + x^3"
+    },
     {
       "input": "1f + 98i + 3f + 14t",
       "output": "1f + (98i + 3f) + 14t",
       "target": "1f + 98i"
     },
     {
-      "input": "8x^2 * (9b) * 7",
+      "input": "8x^2 * (9b * 7)",
       "output": "(8x^2 * 9b) * 7",
       "target": "9b * 7"
     },
@@ -31,4 +36,4 @@
       "input": "2"
     }
   ]
-}
+}

libraries/mathy_python/tests/rules/commutative_property.json

@@ -1,9 +1,71 @@
 {
   "valid": [
-    { 
+    {
+      "input": "2x + 1y^3 + 7j + -2q + 93m + 6x",
+      "target": "2x + 1y^3 + 7j + -2q + 93m + 6x",
+      "output": "2x + 1y^3 + 7j + -2q + 6x + 93m",
+      "why": "use commute to group like terms step 1"
+    },
+    {
+      "input": "2x + 1y^3 + 7j + -2q + 6x + 93m",
+      "target": "2x + 1y^3 + 7j + -2q + 6x",
+      "output": "2x + 1y^3 + 7j + 6x + -2q + 93m",
+      "why": "use commute to group like terms step 2"
+    },
+    {
+      "input": "2x + 1y^3 + 7j + 6x + -2q + 93m",
+      "target": "2x + 1y^3 + 7j + 6x",
+      "output": "2x + 1y^3 + 6x + 7j + -2q + 93m",
+      "why": "use commute to group like terms step 3"
+    },
+    {
+      "input": "2x + 1y^3 + 6x + 7j + -2q + 93m",
+      "target": "2x + 1y^3 + 6x",
+      "output": "2x + 6x + 1y^3 + 7j + -2q + 93m",
+      "why": "use commute to group like terms step 4"
+    },
+    {
+      "input": "12x * 10y",
+      "output": "(x * 12) * 10y",
+      "args": {
+        "preferred": false
+      }
+    },
+    {
+      "input": "4x^3 + g + 5y + 7p^4 + x^3 + 12p^4",
+      "target": "4x^3",
+      "output": "x^3 * 4 + g + 5y + 7p^4 + x^3 + 12p^4"
+    },
+    {
+      "input": "4x^3 + g + 5y + 7p^4 + x^3 + 12p^4",
+      "target": "4x^3 + g",
+      "output": "g + 4x^3 + 5y + 7p^4 + x^3 + 12p^4"
+    },
+    {
+      "input": "4x^3 + g + 5y + 7p^4 + x^3 + 12p^4",
+      "target": "4x^3 + g + 5y",
+      "output": "4x^3 + 5y + g + 7p^4 + x^3 + 12p^4"
+    },
+    {
+      "input": "4x^3 + g + 5y + 7p^4 + x^3 + 12p^4",
+      "target": "4x^3 + g + 5y + 7p^4",
+      "output": "4x^3 + g + 7p^4 + 5y + x^3 + 12p^4"
+    },
+    {
+      "input": "4x^3 + g + 5y + 7p^4 + x^3 + 12p^4",
+      "target": "4x^3 + g + 5y + 7p^4 + x^3",
+      "output": "4x^3 + g + 5y + x^3 + 7p^4 + 12p^4"
+    },
+    {
+      "input": "4x^3 + g + 5y + 7p^4 + x^3 + 12p^4",
+      "target": "4x^3 + g + 5y + 7p^4 + x^3 + 12p^4",
+      "output": "4x^3 + g + 5y + 7p^4 + 12p^4 + x^3"
+    },
+    {
       "input": "2530z + 1m + 3.5x + 2z + 8.9c",
       "target": "2530z + 1m + 3.5x",
-      "output": "3.5x + 2530z + 1m + 2z + 8.9c"
+      "output": "2530z + 3.5x + 1m + 2z + 8.9c",
+      "why": "swapping middle children shouldn't introduce paren nesting"
     },
     {
       "input": "(5 + 12) * a",
@@ -18,13 +80,6 @@
         "preferred": false
       }
     },
-    {
-      "input": "12x * 10y",
-      "output": "(x * 12) * 10y",
-      "args": {
-        "preferred": false
-      }
-    },
     {
       "input": "4 + 17",
       "output": "17 + 4"
@@ -66,4 +121,4 @@
       "input": "7 / x"
     }
   ]
-}
+}

libraries/mathy_python/tests/rules/constants_simplify.json

@@ -93,7 +93,6 @@
       "output": "42x^3 + 30x * x * z + x * 48z^2",
       "why": "Because my rule implementation sucks and can't deal with this kind of nesting... should be: can skip siblings to combine constants"
     },
-
     {
       "input": "13f^4 * (5f^4 + 7)",
       "why": "can't simplify nested child because the nested group has a different priority"
@@ -117,4 +116,4 @@
       "input": "x - 2"
     }
   ]
-}
+}

libraries/mathy_python/tests/test_rules.py

@@ -22,49 +22,49 @@
 from ..mathy.testing import init_rule_for_test, run_rule_tests
 
 
-def test_associative_property():
+def test_rules_associative_property():
     def debug(ex):
         pass
 
     run_rule_tests("associative_property", AssociativeSwapRule, debug)
 
 
-def test_commutative_property():
+def test_rules_commutative_property():
     def debug(ex):
         pass
 
     run_rule_tests("commutative_property", CommutativeSwapRule, debug)
 
 
-def test_constants_simplify():
+def test_rules_constants_simplify():
     def debug(ex):
         pass
 
     run_rule_tests("constants_simplify", ConstantsSimplifyRule, debug)
 
 
-def test_distributive_factor_out():
+def test_rules_distributive_factor_out():
     def debug(ex):
         pass
 
     run_rule_tests("distributive_factor_out", DistributiveFactorOutRule, debug)
 
 
-def test_distributive_multiply_across():
+def test_rules_distributive_multiply_across():
     def debug(ex):
         pass
 
     run_rule_tests("distributive_multiply_across", DistributiveMultiplyRule, debug)
 
 
-def test_variable_multiply():
+def test_rules_variable_multiply():
     def debug(ex):
         pass
 
     run_rule_tests("variable_multiply", VariableMultiplyRule, debug)
 
 
-def test_rule_can_apply_to():
+def test_rules_rule_can_apply_to():
     parser = ExpressionParser()
     expression = parser.parse("7 + 4x - 2")
 
@@ -78,7 +78,7 @@ def test_rule_can_apply_to():
         assert type(action.can_apply_to(expression)) == bool
 
 
-def test_like_terms_compare():
+def test_rules_like_terms_compare():
     parser = ExpressionParser()
     expr = parser.parse("10 + (7x + 6x)")
     terms = get_terms(expr)