-
Notifications
You must be signed in to change notification settings - Fork 51
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
Capture Cplex specific errors and raise a unified optlang.SolverError
[WIP]
#94
Conversation
This introduces the unified optlang error that should be reraised anywhere that solver specific errors can occur. Third party code can then rely on catching the optlang error only.
Using `future` reliably allows us to reraise in Python 2 & 3.
As a basis AbstractDualValuesMIPTestCase expects dual values access of any shape or form to raise a SolverError exception for now. This will change in future and only Cplex raises. Also implemented specific Cplex test cases InfeasibleMIPTestCase that test an infeasible MIP and expect certain kinds of errors.
optlang/cplex_interface.py
Outdated
@@ -31,14 +31,17 @@ | |||
|
|||
log = logging.getLogger(__name__) | |||
|
|||
from future.utils import raise_from |
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.
I think we should stick with the six module instead of using two different compatibility libraries.
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.
future
is built on top of six and I find it a lot nicer in many cases but sure, I can drop it.
@@ -497,6 +499,98 @@ def test_qp_non_convex(self): | |||
model.optimize() | |||
self.assertAlmostEqual(model.objective.value, 2441.999999971) | |||
|
|||
class InfeasibleMIPTestCase(unittest.TestCase): |
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.
Couldn't this be combined with the other test case?
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.
I could drop those cases that are tested by accessing dual values in MIP already, true. However, I want to test here that all Cplex errors are adequately caught and reraised as optlang.SolverError
s. That behavior is different between MIPs and LPs. Maybe I should extend this to LPs and QPs, too.
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.
I agree that it's good to test this as well, and I just meant that these tests could simply be added to the other test case.
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.
I'd rather keep them separate since the intention of the tests is something different.
Codecov Report
@@ Coverage Diff @@
## devel #94 +/- ##
=========================================
- Coverage 88.8% 88.63% -0.18%
=========================================
Files 8 8
Lines 2386 2419 +33
Branches 542 542
=========================================
+ Hits 2119 2144 +25
- Misses 166 174 +8
Partials 101 101
Continue to review full report at Codecov.
|
optlang.SolverError
optlang.SolverError
[WIP]
Closing due to #96 which is a polished version of this branch. |
When working with different optlang solver interfaces error handling should be agnostic. So far this is implemented for Cplex and Gurobi should come next. As far as I'm aware GLPK raises no errors but maybe surprising behavior should still be handled by raising from optlang. I'm not sure.
So far this pull request also implements test cases for accessing dual values on MIP problems. These should go into a separate branch/PR, I suppose.
Tests cannot pass at the moment since I introduce two failing tests due to the above.