Conversation
blegat
force-pushed
the
bl/bridge_dual_test
branch
from
359a8e3 to
cded928
Compare
|
I'm still against this. Bridges could/should write better tests regardless. Testing the dual is tricky. And even our |
|
Yes, we just check that get and set are inverse of each other and that's precisely what this PR is fixing. Now it also checks that it is the adjoint of the bridge transformation so it's starting to get difficult to get the test passing with an incorrect dual |
odow
force-pushed
the
bl/bridge_dual_test
branch
from
cded928 to
2553c6c
Compare
src/Utilities/results.jl
Outdated
| ci::MOI.ConstraintIndex{ | ||
| <:MOI.AbstractScalarFunction, | ||
| <:Union{MOI.ZeroOne,MOI.Interval}, | ||
| }, |
There was a problem hiding this comment.
This needs to be a separate PR. What is ZeroOne doing in a PR about testing constraint duals?
There was a problem hiding this comment.
This is an issue detected by running the dual tests through all bridges. This is therefore tested in this PR as well
There was a problem hiding this comment.
Which bridge? Why is ZeroOne involved? How can there be a dual if the ZeroOne set is involved?
There was a problem hiding this comment.
The AbstractFunctionConversionBridge already supports setting ConstraintDual, they treat it the same as ConstraintDualStart so it got hit by the new tests.
How can there be a dual if the ZeroOne set is involved?
From the perspective of _dual_objective_value, since ZeroOne is equivalent to Interval + Integer, and Integer is ignored by _dual_objective_value then it's ok to treat ZeroOne as an Interval. If the solver doesn't have any dual, we will fail when we query the dual from the solver anyway with the expected error. Here, at the level of bridges or _dual_objective_value, we are just taking the transpose of linear maps so we shouldn't worry about whether some variables are integer or not.
There was a problem hiding this comment.
Can this be a separate PR? (With an explicit test)
There was a problem hiding this comment.
We can disable the tests for the failing ones yes
|
Let me take over from here and split it up. |
odow
force-pushed
the
bl/bridge_dual_test
branch
from
810361e to
7224166
Compare
|
So you added This design feels wrong. Could dual tests be opt-in? Is a separate bug that we don't have a generic fallback for |
|
#2826 removes the use of |
odow
force-pushed
the
bl/bridge_dual_test
branch
from
bdcbbd2 to
cfcbc5d
Compare
| # If the bridges does not support `ConstraintDualStart`, it probably won't | ||
| # support `ConstraintDual` so we skip these tests | ||
| list_of_constraints = MOI.get(model, MOI.ListOfConstraintTypesPresent()) | ||
| attr = MOI.ConstraintDual() |
There was a problem hiding this comment.
Did you mean
| attr = MOI.ConstraintDual() | |
| attr = MOI.ConstraintDualStart() |
There was a problem hiding this comment.
No, if the bridge don't support ConstraintDual, we skip the test. Because get_fallback don't work with ConstraintDualStart, we need to use ConstraintDual
odow
force-pushed
the
bl/bridge_dual_test
branch
from
cfcbc5d to
b4d6987
Compare
2 participants
I always thought it a bit silly to only define the setter for
ConstraintDualStartand not withConstraintDual. I sometimes want to test things mixing aMockOptimizerand a bridge and I can't because bridges don't define the setters forConstraintDual.I suggest the following non-breaking change: We should now encourage bridges to supports
ConstraintDual. When they do, a new test will be enabled that will do an important consistency check. So the new test will not be run for existing bridge which is nice since it could be breaking but after the transition, it will be run for all bridges that don't disable it by doingdual = nothing.This would have caught the bug in #2809 as the CI should show