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.

Sign up for GitHub

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

Fix promote_operation for ScalarNonlinearFunction #2179

Merged
merged 9 commits into from
May 31, 2023
Merged

Fix promote_operation for ScalarNonlinearFunction #2179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4893d57
Fix promote_operation for ScalarNonlinearFunction
odow May 29, 2023
35f537e
Skip vectorize test for SNF
odow May 29, 2023
89a69a8
More fixes
odow May 29, 2023
2e5e879
Fix formattinng
odow May 29, 2023
f66e799
Update ci.yml
odow May 30, 2023
44f019e
Update functions.jl
odow May 30, 2023
b2633a0
Update
odow May 30, 2023
b706216
Update
odow May 30, 2023
62c8261
Apply suggestions from code review
odow May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Bridges/Constraint/bridges/vectorize.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ function MOI.supports_constraint(
return true
end

function MOI.supports_constraint(
::Type{VectorizeBridge{T}},
::Type{MOI.ScalarNonlinearFunction},
::Type{<:MOI.Utilities.ScalarLinearSet{T}},
) where {T}
return false
end

function MOI.Bridges.added_constrained_variable_types(::Type{<:VectorizeBridge})
return Tuple{Type}[]
end
Expand Down
44 changes: 36 additions & 8 deletions src/Utilities/functions.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1817,28 +1817,56 @@ end
### ScalarNonlinearFunction

function promote_operation(
::typeof(-),
::Type{T},
::Union{typeof(+),typeof(-)},
::Type{<:Number},
::Type{MOI.ScalarNonlinearFunction},
)
return MOI.ScalarNonlinearFunction
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blegat: so one issue. A lot of the bridges are very permissive in the functions that accept. And then they use promote_operation to figure out what the resultant function is. That makes it hard for new functions, because we need to add a bunch of these promote_operation methods. An alternative would be to make the bridge explicitly declare what functions they support, and external packages would need to opt-into them. Not something we necessarily need to change now, but another MOI 2.0 list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having to opt-in for each bridge would loose a lot of the bridges advantages. Right now, the fact that we will be able to no nonlinear conic with the bridges just by implementing a few MOI.Utilities is quite appealing.
I can understand that when you create a new function you would like to be able to test it without having to do all these things.
One thing you could do is to use MOI.Bridges.remove_bridge with the bridges that are annoying you.
We could have a way to say that an operation is not supported as well so that you could do that and then not have to implement MOI.Utilities.operate, etc...


function operate(
::typeof(-),
::Type{T},
f::MOI.ScalarNonlinearFunction,
) where {T}
return MOI.ScalarNonlinearFunction
if f.head == :- && length(f.args) == 1
# A simplification for -(-(f)) into f, but only if f is an SNF.
if f.args[1] isa MOI.ScalarNonlinearFunction
return f.args[1]
end
end
return MOI.ScalarNonlinearFunction(:-, Any[f])
end

function promote_operation(
::typeof(-),
::Union{typeof(+),typeof(-),typeof(*),typeof(/)},
::Type{T},
::Type{MOI.ScalarNonlinearFunction},
::Type{MOI.VariableIndex},
) where {T}
::Type{S},
) where {
T<:Number,
S<:Union{
T,
MOI.VariableIndex,
MOI.ScalarAffineFunction{T},
MOI.ScalarQuadraticFunction{T},
MOI.ScalarNonlinearFunction,
},
}
return MOI.ScalarNonlinearFunction
end

function operate(
op::Union{typeof(+),typeof(-)},
op::Union{typeof(+),typeof(-),typeof(*),typeof(/)},
::Type{T},
f::MOI.ScalarNonlinearFunction,
g::ScalarQuadraticLike{T},
g::Union{
T,
MOI.VariableIndex,
MOI.ScalarAffineFunction{T},
MOI.ScalarQuadraticFunction{T},
MOI.ScalarNonlinearFunction,
},
) where {T}
return MOI.ScalarNonlinearFunction(Symbol(op), Any[f, g])
end
Expand Down
22 changes: 22 additions & 0 deletions test/Bridges/Constraint/flip_sign.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,17 @@ function test_runtests()
-1.5 * x <= -1.0
""",
)
MOI.Bridges.runtests(
MOI.Bridges.Constraint.GreaterToLessBridge,
"""
variables: x
ScalarNonlinearFunction(1.5 * x) >= 1.0
""",
"""
variables: x
ScalarNonlinearFunction(-(1.5 * x)) <= -1.0
""",
)
MOI.Bridges.runtests(
MOI.Bridges.Constraint.LessToGreaterBridge,
"""
Expand All @@ -359,6 +370,17 @@ function test_runtests()
-1.5 * x >= -1.0
""",
)
MOI.Bridges.runtests(
MOI.Bridges.Constraint.LessToGreaterBridge,
"""
variables: x
ScalarNonlinearFunction(1.5 * x) <= 1.0
""",
"""
variables: x
ScalarNonlinearFunction(-(1.5 * x)) >= -1.0
""",
)
MOI.Bridges.runtests(
MOI.Bridges.Constraint.NonnegToNonposBridge,
"""
Expand Down
22 changes: 22 additions & 0 deletions test/Bridges/Constraint/vectorize.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,28 @@ function test_runtests()
return
end

MOI.Utilities.@model(
Model2179,
(),
(MOI.GreaterThan, MOI.LessThan),
(),
(),
(),
(MOI.ScalarAffineFunction,),
(),
()
)

function test_unsupported_ScalarNonlinearFunction()
model = MOI.instantiate(Model2179{Float64}; with_bridge_type = Float64)
MOI.supports_constraint(
model,
MOI.ScalarNonlinearFunction,
MOI.GreaterThan{Float64},
)
odow marked this conversation as resolved.
Show resolved Hide resolved
return
end

end # module

TestConstraintVectorize.runtests()