Repository navigation
Conversation
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 |
cded928 to
2553c6c
Compare
|
Let me take over from here and split it up. |
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 |
bdcbbd2 to
cfcbc5d
Compare
cfcbc5d to
b4d6987
Compare
b4d6987 to
c8ecc94
Compare
| ::MOI.ConstraintDual, | ||
| ::Type{MOI.ConstraintIndex{F,S}}, | ||
| ) where {F,S} | ||
| return !(S <: Union{MOI.ZeroOne,MOI.Integer,MOI.SOS1,MOI.SOS2}) |
There was a problem hiding this comment.
Because otherwise some of the bridge tests fail with weird errors. I should take another look.
| ::MOI.ConstraintDual, | ||
| ::Type{<:AbstractFunctionConversionBridge{F,S}}, | ||
| ) where {F,S} | ||
| return false |
There was a problem hiding this comment.
Why don't we support it ? The mapping is just the identity so we can map it identically
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