Remove bridge instead of test #317
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #317 +/- ##
==========================================
+ Coverage 89.07% 89.08% +0.01%
==========================================
Files 16 16
Lines 2077 2079 +2
==========================================
+ Hits 1850 1852 +2
Misses 227 227 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # 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 |
There was a problem hiding this comment.
Why would we need ConstraintFunction ? I don't understand this one
There was a problem hiding this comment.
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
There was a problem hiding this comment.
DiffOpt needs a solver that supports ConstraintFuncion because it will copy those constraints from the optimizer to the .diff model
| 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 |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
We can just say that in DiffOpt we know we want the duals so the case is clear
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I will change the comment
There was a problem hiding this comment.
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.
2 participants
If there is a follow-up to jump-dev/MathOptInterface.jl#2863 removing the test we can remove this.
At the same time its does not seem bad to block Zeros bridge from start