refactor: deprecate combine_operators -> combine_operators!

Author: MilesCranmer

src/PatchMethods.jl

@@ -5,15 +5,15 @@ using ..OperatorEnumModule: AbstractOperatorEnum
 using ..NodeModule: constructorof
 using ..ExpressionModule: Expression, get_tree, get_operators
 using ..ParametricExpressionModule: ParametricExpression
-import ..SimplifyModule: combine_operators, simplify_tree!
+import ..SimplifyModule: combine_operators!, simplify_tree!
 
 # Avoid implementing a generic version for these, as it is less likely to generalize
-function combine_operators(
+function combine_operators!(
     ex::Union{Expression{T,N},ParametricExpression{T,N}},
     operators::Union{AbstractOperatorEnum,Nothing}=nothing,
 ) where {T,N}
     return with_contents(
-        ex, combine_operators(get_contents(ex), get_operators(ex, operators))
+        ex, combine_operators!(get_contents(ex), get_operators(ex, operators))
     )
 end
 function simplify_tree!(

src/Simplify.jl

@@ -33,29 +33,29 @@ has_identity_one(_) = false
 simplifies_given_equal_operands(::typeof(/)) = true
 simplifies_given_equal_operands(_) = false
 
-combine_operators(tree::AbstractExpressionNode, ::AbstractOperatorEnum) = tree
+combine_operators!(tree::AbstractExpressionNode, ::AbstractOperatorEnum) = tree
 
-function combine_operators(tree::Node{T}, operators::AbstractOperatorEnum) where {T}
+function combine_operators!(tree::Node{T}, operators::AbstractOperatorEnum) where {T}
     deg = tree.degree
     deg == 0 && return tree
-    tree.l = combine_operators(tree.l, operators)
+    tree.l = combine_operators!(tree.l, operators)
     deg == 1 && return tree
-    tree.r = combine_operators(tree.r, operators)
-    return dispatch_deg2_simplify(tree, operators)
+    tree.r = combine_operators!(tree.r, operators)
+    return dispatch_deg2_simplify!(tree, operators)
 end
-@generated function dispatch_deg2_simplify(
+@generated function dispatch_deg2_simplify!(
     tree::Node{T}, operators::AbstractOperatorEnum
 ) where {T}
     nbin = get_nbin(operators)
     quote
         op_idx = tree.op
         return Base.Cartesian.@nif(
-            $nbin, i -> i == op_idx, i -> _combine_operators_on(operators.binops[i], tree)
+            $nbin, i -> i == op_idx, i -> _combine_operators_on!(tree, operators.binops[i])
         )
     end
 end
 
-function _combine_operators_on(f::F, tree::Node{T}) where {F,T}
+function _combine_operators_on!(tree::Node{T}, f::F) where {F,T}
     # NOTE: This assumes tree.degree == 2 and tree.op corresponds to f
     # Handle basic simplifications first
     if is_node_constant(tree.r)

src/deprecated.jl

@@ -2,6 +2,7 @@ import Base: @deprecate
 import .NodeModule: Node, GraphNode
 
 @deprecate simplify_tree(tree, operators) simplify_tree!(tree, operators)
+@deprecate combine_operators(tree, operators) combine_operators!(tree, operators)
 
 for N in (:Node, :GraphNode)
     @eval begin

test/test_simplification.jl

@@ -94,12 +94,12 @@ end
         Node(1, Node(1, Node(; val=0.1), Node(; val=0.2)) + Node(; val=0.2)) +
         Node(; val=2.0)
     @test repr(tree) ≈ "(cos((0.1 + 0.2) + 0.2) + 2.0)"
-    @test repr(combine_operators(tree, operators)) ≈ "(cos(0.4 + 0.1) + 2.0)"
+    @test repr(combine_operators!(tree, operators)) ≈ "(cos(0.4 + 0.1) + 2.0)"
 end
 
 @testitem "Basic operator simplifications" begin
     using DynamicExpressions, Test
-    import DynamicExpressions.SimplifyModule: combine_operators
+    import DynamicExpressions.SimplifyModule: combine_operators!
 
     operators = OperatorEnum(; binary_operators=(+, -, *, /), unary_operators=(cos, sin))
     x = Node(; feature=1)
@@ -110,52 +110,52 @@ end
 
     # multiplication by 0
     tree = zero_node * x
-    @test combine_operators(tree, operators) == zero_node
+    @test combine_operators!(tree, operators) == zero_node
     tree = x * zero_node
-    @test combine_operators(tree, operators) == zero_node
+    @test combine_operators!(tree, operators) == zero_node
 
     # multiplication by 1
     tree = one_node * x
-    @test combine_operators(tree, operators) == x
+    @test combine_operators!(tree, operators) == x
     tree = x * one_node
-    @test combine_operators(tree, operators) == x
+    @test combine_operators!(tree, operators) == x
 
     # addition by 0
     tree = zero_node + x
-    @test combine_operators(tree, operators) == x
+    @test combine_operators!(tree, operators) == x
     tree = x + zero_node
-    @test combine_operators(tree, operators) == x
+    @test combine_operators!(tree, operators) == x
 
     # division by self -> 1
     tree = x / x
-    @test combine_operators(tree, operators).val == 1.0
+    @test combine_operators!(tree, operators).val == 1.0
 
     # nested multiplication by constants
     tree1 = (two_node * x) * three_node
     tree2 = Node(; val=6.0) * x
-    @test combine_operators(tree1, operators) == combine_operators(tree2, operators)
+    @test combine_operators!(tree1, operators) == combine_operators!(tree2, operators)
 end
 
 @testitem "Constant combination" begin
     using DynamicExpressions, Test
-    import DynamicExpressions.SimplifyModule: combine_operators
+    import DynamicExpressions.SimplifyModule: combine_operators!
 
     operators = OperatorEnum(; binary_operators=(+, -, *, /), unary_operators=(cos, sin))
     x1 = Node(; feature=1)
 
     # Test commutative constant combination
     tree = Node(; val=0.5) + (Node(; val=0.2) + x1)
-    @test combine_operators(tree, operators) == x1 + Node(; val=0.7)
+    @test combine_operators!(tree, operators) == x1 + Node(; val=0.7)
 
     # Test nested multiplication by constants
     tree = (Node(; val=2.0) * x1) * Node(; val=3.0)
-    @test combine_operators(tree, operators) == x1 * Node(; val=6.0)
+    @test combine_operators!(tree, operators) == x1 * Node(; val=6.0)
 
     # Test nested addition by constants
     tree = (Node(; val=2.0) + x1) + Node(; val=3.0)
-    @test combine_operators(tree, operators) == x1 + Node(; val=5.0)
+    @test combine_operators!(tree, operators) == x1 + Node(; val=5.0)
 
     # Test mixed operations don't combine incorrectly
     tree = (Node(; val=2.0) * x1) + Node(; val=3.0)
-    @test combine_operators(tree, operators) == tree
+    @test combine_operators!(tree, operators) == tree
 end