Don't copy ScalarNonlinearFunction in VectorOfConstraints

docs/src/submodules/Utilities/reference.md

@@ -179,6 +179,7 @@ The following functions can be used to canonicalize a function:
 ```@docs
 Utilities.is_canonical
 Utilities.canonical
+Utilities.copy_if_modifiable
 Utilities.canonicalize!
 ```
 

docs/src/tutorials/implementing.md

@@ -712,6 +712,13 @@ This applies, for example, to the `terms` vector in
 afterwards. The in-place version of [`get!`](@ref) can be used by users to avoid
 extra copies in this case.
 
+Functions that cannot be modified in-place through the MOI API, such as
+[`ScalarNonlinearFunction`](@ref) and [`VectorNonlinearFunction`](@ref), do not
+need to be copied. Avoid copying them: the same
+[`ScalarNonlinearFunction`](@ref) object may be used several times as a common
+subexpression, and a copy would turn these aliases into distinct objects. Use
+[`Utilities.copy_if_modifiable`](@ref) to copy a function only if needed.
+
 ### Column Generation
 
 There is no special interface for column generation. If the solver has a special

src/Bridges/Constraint/bridges/SplitIntervalBridge.jl

@@ -430,7 +430,7 @@ function MOI.get(
     if bridge.upper !== nothing
         return MOI.get(model, attr, bridge.upper)
     end
-    return copy(bridge.func)
+    return MOI.Utilities.copy_if_modifiable(bridge.func)
 end
 
 function MOI.get(

src/Utilities/functions.jl

@@ -1081,8 +1081,9 @@ Returns the function in a canonical form, that is,
  * For a `AbstractVectorFunction`, the terms are sorted in ascending order of
    output index.
 
-The output of `canonical` can be assumed to be a copy of `f`, even for
-`VectorOfVariables`.
+If `f` can be modified in-place through the MOI API, the output of `canonical`
+can be assumed to be a copy of `f`. Otherwise, `f` is returned if it is already
+canonical, see [`copy_if_modifiable`](@ref).
 
 ## Example
 
@@ -1100,15 +1101,50 @@ julia> MOI.Utilities.canonical(f)
 ```
 """
 function canonical(f::MOI.AbstractFunction)
-    g = copy(f)
-    if !is_canonical(g)
-        canonicalize!(g)
+    if is_canonical(f)
+        return copy_if_modifiable(f)
     end
-    return g
+    return canonicalize!(copy(f))
 end
 
 canonicalize!(f::Union{MOI.VectorOfVariables,MOI.VariableIndex}) = f
 
+"""
+    copy_if_modifiable(f::MOI.AbstractFunction)
+
+Return a copy of `f` if `f` can be modified in-place through the MOI API, for
+example, with [`MOI.modify`](@ref). Otherwise, return `f`.
+
+A model should call this function when it stores a function given by the user,
+for example, in [`MOI.add_constraint`](@ref), and when it returns a function that
+it stores, for example, for [`MOI.ConstraintFunction`](@ref). Otherwise,
+modifying the function of the model through the MOI API would also modify the
+function of the user.
+
+The default implementation returns `copy(f)`. Functions that cannot be modified
+in-place through the MOI API, such as `MOI.VariableIndex`,
+`MOI.VectorOfVariables`, `MOI.ScalarNonlinearFunction` and
+`MOI.VectorNonlinearFunction`, return `f`. If you define a new function type `F`
+that cannot be modified in-place through the MOI API, implement
+`copy_if_modifiable(f::F) = f`.
+
+Not copying is important for `MOI.ScalarNonlinearFunction`: the same object may
+be used several times as a common subexpression, and a copy would turn these
+aliases into distinct objects.
+"""
+copy_if_modifiable(f::MOI.AbstractFunction) = copy(f)
+
+function copy_if_modifiable(
+    f::Union{
+        MOI.VariableIndex,
+        MOI.VectorOfVariables,
+        MOI.ScalarNonlinearFunction,
+        MOI.VectorNonlinearFunction,
+    },
+)
+    return f
+end
+
 """
     canonicalize!(f::Union{ScalarAffineFunction, VectorAffineFunction})
 

src/Utilities/objective_container.jl

@@ -167,7 +167,7 @@ function MOI.set(
 )
     _empty_keeping_sense(o)
     o.is_function_set = true
-    o.single_variable = copy(f)
+    o.single_variable = copy_if_modifiable(f)
     return
 end
 
@@ -178,7 +178,7 @@ function MOI.set(
 ) where {T}
     _empty_keeping_sense(o)
     o.is_function_set = true
-    o.scalar_affine = copy(f)
+    o.scalar_affine = copy_if_modifiable(f)
     return
 end
 
@@ -189,7 +189,7 @@ function MOI.set(
 ) where {T}
     _empty_keeping_sense(o)
     o.is_function_set = true
-    o.scalar_quadratic = copy(f)
+    o.scalar_quadratic = copy_if_modifiable(f)
     return
 end
 
@@ -200,7 +200,7 @@ function MOI.set(
 )
     _empty_keeping_sense(o)
     o.is_function_set = true
-    o.scalar_nonlinear = copy(f)
+    o.scalar_nonlinear = copy_if_modifiable(f)
     return
 end
 
@@ -211,7 +211,7 @@ function MOI.set(
 )
     _empty_keeping_sense(o)
     o.is_function_set = true
-    o.vector_variables = copy(f)
+    o.vector_variables = copy_if_modifiable(f)
     return
 end
 
@@ -222,7 +222,7 @@ function MOI.set(
 ) where {T}
     _empty_keeping_sense(o)
     o.is_function_set = true
-    o.vector_affine = copy(f)
+    o.vector_affine = copy_if_modifiable(f)
     return
 end
 
@@ -233,7 +233,7 @@ function MOI.set(
 ) where {T}
     _empty_keeping_sense(o)
     o.is_function_set = true
-    o.vector_quadratic = copy(f)
+    o.vector_quadratic = copy_if_modifiable(f)
     return
 end
 
@@ -244,7 +244,7 @@ function MOI.set(
 )
     _empty_keeping_sense(o)
     o.is_function_set = true
-    o.vector_nonlinear = copy(f)
+    o.vector_nonlinear = copy_if_modifiable(f)
     return
 end
 

src/Utilities/universalfallback.jl

@@ -582,7 +582,7 @@ function MOI.set(
         # Clear any fallback objective
         uf.objective = nothing
     else
-        uf.objective = copy(func)
+        uf.objective = copy_if_modifiable(func)
         # Clear any `model` objective
         sense = MOI.get(uf.model, MOI.ObjectiveSense())
         MOI.set(uf.model, MOI.ObjectiveSense(), MOI.FEASIBILITY_SENSE)

src/Utilities/vector_of_constraints.jl

@@ -103,7 +103,7 @@ function MOI.get(
 ) where {F,S}
     MOI.throw_if_not_valid(v, ci)
     f, _ = v.constraints[ci]::Tuple{F,S}
-    return copy(f)
+    return copy_if_modifiable(f)
 end
 
 function MOI.get(

src/functions.jl

@@ -311,6 +311,21 @@ Each element must be one of the following:
  * A [`ScalarQuadraticFunction`](@ref)
  * A [`ScalarNonlinearFunction`](@ref)
 
+## Copies
+
+A `ScalarNonlinearFunction` cannot be modified in-place through the MOI API, so
+models do not copy it when it is passed to [`add_constraint`](@ref) or set as
+the [`ObjectiveFunction`](@ref), and they may return the same object when it is
+queried with [`ConstraintFunction`](@ref) or [`ObjectiveFunction`](@ref), see
+[`Utilities.copy_if_modifiable`](@ref). Therefore, do not modify a
+`ScalarNonlinearFunction` after giving it to a model or getting it from a model.
+
+This allows sharing a common subexpression by using the same
+`ScalarNonlinearFunction` object several times, in the expression graph of one
+function or of several functions of the model. Copying would replace these
+aliases by distinct objects, which would prevent `Nonlinear.ReverseAD`
+from detecting them as common subexpressions.
+
 ## Unsupported operators
 
 If the optimizer does not support `head`, an [`UnsupportedNonlinearOperator`](@ref)
@@ -682,6 +697,11 @@ following:
  * A [`ScalarQuadraticFunction`](@ref)
  * A [`ScalarNonlinearFunction`](@ref)
 
+## Copies
+
+Like [`ScalarNonlinearFunction`](@ref), models do not copy a
+`VectorNonlinearFunction`, see [`Utilities.copy_if_modifiable`](@ref).
+
 ## Example
 
 To represent the function ``f(x) = [sin(x)^2, x]``, do:

src/indextypes.jl

@@ -19,6 +19,18 @@ All functions must implement:
 
 Abstract subtypes of `AbstractFunction` may require additional methods to be
 implemented.
+
+## Copies
+
+Models only copy the functions that can be modified in-place through the MOI
+API, for example, with [`modify`](@ref), using
+[`Utilities.copy_if_modifiable`](@ref). For example,
+[`ScalarAffineFunction`](@ref) is copied but [`ScalarNonlinearFunction`](@ref)
+is not.
+
+If you define a new function type that cannot be modified in-place through the
+MOI API, implement [`Utilities.copy_if_modifiable`](@ref) so that it is not
+copied.
 """
 abstract type AbstractFunction <: MA.AbstractMutable end
 

test/Utilities/test_functions.jl

@@ -2101,6 +2101,36 @@ function test_ScalarNonlinearFunction_is_canonical()
     return
 end
 
+function test_canonical_copy_if_modifiable()
+    x = MOI.VariableIndex(1)
+    for f in (
+        1.0 * x + 2.0,
+        1.0 * x * x + 2.0,
+        MOI.Utilities.vectorize([1.0 * x + 2.0]),
+        MOI.Utilities.vectorize([1.0 * x * x + 2.0]),
+    )
+        @test MOI.Utilities.is_canonical(f)
+        g = MOI.Utilities.canonical(f)
+        @test g ≈ f
+        @test g !== f
+        g = MOI.Utilities.copy_if_modifiable(f)
+        @test g ≈ f
+        @test g !== f
+    end
+    for f in (
+        x,
+        MOI.VectorOfVariables([x]),
+        MOI.ScalarNonlinearFunction(:sin, Any[x]),
+        MOI.VectorNonlinearFunction([
+            MOI.ScalarNonlinearFunction(:sin, Any[x]),
+        ]),
+    )
+        @test MOI.Utilities.canonical(f) === f
+        @test MOI.Utilities.copy_if_modifiable(f) === f
+    end
+    return
+end
+
 function test_vector_type()
     for T in (Int, Float64)
         @test MOI.Utilities.vector_type(T) == Vector{T}

test/Utilities/test_model.jl

@@ -619,6 +619,30 @@ end
 
 MOI.Utilities.@model(EmptyModel, (), (), (), (), (), (), (), ())
 
+function test_ScalarNonlinearFunction_not_copied()
+    model = MOI.Utilities.Model{Float64}()
+    x = MOI.add_variable(model)
+    sin_x = MOI.ScalarNonlinearFunction(:sin, Any[x])
+    f = MOI.ScalarNonlinearFunction(:+, Any[sin_x, sin_x])
+    g = MOI.ScalarNonlinearFunction(:*, Any[sin_x, x])
+    cf = MOI.add_constraint(model, f, MOI.EqualTo(0.0))
+    cg = MOI.add_constraint(model, g, MOI.EqualTo(0.0))
+    h = MOI.VectorNonlinearFunction([f, g])
+    ch = MOI.add_constraint(model, h, MOI.Zeros(2))
+    MOI.set(model, MOI.ObjectiveFunction{typeof(f)}(), f)
+    @test MOI.get(model, MOI.ConstraintFunction(), cf) === f
+    @test MOI.get(model, MOI.ConstraintFunction(), cg) === g
+    @test MOI.get(model, MOI.ConstraintFunction(), ch) === h
+    @test MOI.get(model, MOI.ObjectiveFunction{typeof(f)}()) === f
+    dest = MOI.Utilities.Model{Float64}()
+    index_map = MOI.copy_to(dest, model)
+    f_dest = MOI.get(dest, MOI.ConstraintFunction(), index_map[cf])
+    g_dest = MOI.get(dest, MOI.ConstraintFunction(), index_map[cg])
+    @test f_dest.args[1] === f_dest.args[2]
+    @test f_dest.args[1] === g_dest.args[1]
+    return
+end
+
 function test_empty_model()
     model = EmptyModel{Float64}()
     @test MOI.is_empty(model)