Skip to content

Remove bridge instead of test #317

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove bridge instead of test #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions src/moi_wrapper.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,19 @@ function diff_optimizer(
with_bridge_type,
with_cache_type,
)
if !isnothing(with_bridge_type)
# removed because of the `ZerosBridge` issue:
# https://github.com/jump-dev/MathOptInterface.jl/issues/2861
# - zeros bridge does not support duals because it cumbersome
Copy link
Member

Choose a reason for hiding this comment

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

cumbersome ? Shouldn't we simply say that ZerosBridge does not support dual and we know with DiffOpt that we will need the duals so we can't use that bridge ?

Copy link
Member

Choose a reason for hiding this comment

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

This bridge is only useful in situations where we don't need the dual so in case we are sure in advance that we need the dual it's clear we can remove it

Copy link
Member

Choose a reason for hiding this comment

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

We can just say that in DiffOpt we know we want the duals so the case is clear

Copy link
Member Author

Choose a reason for hiding this comment

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

One issue is, if the bridge is there, we allow the user to pass some constraints but then we disallow duals for those.
An MOI test fails and it is conceivable that someone hits this wall.
One idea is to not add that constraint, hence the removal of the bridge.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will change the comment

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, then it matches what I said, we should just say that this bridge is enabled by default and MOI expects the user to remove if it is known in advance that duals will be needed. Since we know we want duals here, we know we can remove it in advance.

# - many bridges do not support get ConstraintFunction because it is cumbersome
Copy link
Member

Choose a reason for hiding this comment

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

Why would we need ConstraintFunction ? I don't understand this one

Copy link
Member

Choose a reason for hiding this comment

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

This is something needed by the tests but in practice, no one asks for ConstraintFunction to the bridges outside of the tests. It's not clear in your comment if DiffOpt needs to query ConstraintFunction or not

Copy link
Member Author

Choose a reason for hiding this comment

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

DiffOpt needs a solver that supports ConstraintFuncion because it will copy those constraints from the optimizer to the .diff model

# so there is no way out of this error for now.
# at the same time this is a modeling corner case tha could be avoided
# by the user.
MOI.Bridges.remove_bridge(
optimizer,
MOI.Bridges.Variable.ZerosBridge{with_bridge_type},
)
end
if allow_parametric_opt_interface &&
!MOI.supports_add_constrained_variable(
isnothing(with_bridge_type) ? optimizer : optimizer.model,
Expand Down
19 changes: 1 addition & 18 deletions test/moi_wrapper.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,27 +27,10 @@ end

function test_moi_test_runtests()
model = DiffOpt.diff_optimizer(HiGHS.Optimizer)
# `Variable.ZerosBridge` makes dual needed by some tests fail.
# MOI.Bridges.remove_bridge(
# model.optimizer.optimizer.optimizer,
# MOI.Bridges.Variable.ZerosBridge{Float64},
# )
MOI.set(model, MOI.Silent(), true)
config =
MOI.Test.Config(; atol = 1e-7, exclude = Any[MOI.compute_conflict!,])
MOI.Test.runtests(
model,
config;
exclude = Any[
# removed because of the `ZerosBridge` issue:
# https://github.com/jump-dev/MathOptInterface.jl/issues/2861
# - zeros bridge does not support duals because it cumbersome
# - many bridges do not support get ConstraintFunction because it is cumbersome
# so there is no way out of this error for now.
# at the same time this is a modeling corner case tha could be avoided
# by the user.
"test_conic_linear_VectorOfVariables_2"],
)
MOI.Test.runtests(model, config;)
return
end

Expand Down