-
Notifications
You must be signed in to change notification settings - Fork 92
Test ConstraintDual for bridges #2810
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.
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
base: master
Are you sure you want to change the base?
Conversation
359a8e3
to
cded928
Compare
attr::Union{ | ||
MOI.ConstraintPrimalStart, | ||
MOI.ConstraintDualStart, | ||
MOI.ConstraintDual, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big dislike from me. ConstraintDual is a result attribute. We shouldn't make bridges "support" it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since MockOptimizer
supports ConstraintDual
, then it's quite useful for testing. ConstraintDual
and ConstraintDualStart
have different semantics for solvers but for bridges that only care about how things are transformed, they are completely equivalent.
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 |
I always thought it a bit silly to only define the setter for
ConstraintDualStart
and not withConstraintDual
. I sometimes want to test things mixing aMockOptimizer
and 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