-
Notifications
You must be signed in to change notification settings - Fork 37
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
Handle infeasibility in branch and price #25
base: development
Are you sure you want to change the base?
Conversation
Maven test passes when including these changes.
@@ -264,8 +275,10 @@ public void solve(long timeLimit) | |||
if (hasNewCuts) | |||
continue; | |||
else | |||
status = SolverStatus.OPTIMAL; |
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.
the else clause must have brackets { }
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.
fixed
*/ | ||
public boolean isOptimal() | ||
public SolverStatus getStatus() |
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 will break backward compatibility more than it should. Just leave isOptimal() in and add a new method getStatus()
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, will fix this. W\hat about deprecating isOptimal()
in this case? This could simplify future maintenance and makes it clear to the user that he can also use the alternative method?
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.
Fixed with deprecation. Let me know if you rather not see the method deprecated
@@ -0,0 +1,22 @@ | |||
package org.jorlib.frameworks.columngeneration.util; |
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.
please add the common class header (modified with your name and 2017-2017)
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.
Added, I hope that this is how you meant it?
* | ||
* @author Rowan Hoogervorst | ||
*/ | ||
public enum SolverStatus |
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.
We should probably think of a more elaborate return object. For instance, in case the solution is UNDECIDED, it would be nice to know what happened, e.g. 'time limit' or 'iteration limit' or something else. The first thing you want to know is the general status, e.g. infeasible/optimal/undecided/.... After that, you probably want to find out what the cause if of that status.
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 thought of a similar approach but found it difficult to define all possible cases. Namely, it is quite difficult to determine beforehand which possible causes are available for a solution to not be found. Moreover, I had the feeling that such an approach is somewhat orthogonal to the handling of reaching the time limit at the moment, which mostly occurs through means of exceptions.
For this reason, the current separation is mostly through the actual possible states of the solve, where the reaching of a time limit might either lead to status UNDECIDED
in case no solution has be found yet or to SOLUTION_AVAILABLE
in case a feasible solution has been found.
One possible solution would be to add a constructor for the enums, which receives additional reasons for the status. Alternatively, we might simply extend the number of states, like is done by e.g. CPLEX: http://www.ibm.com/support/knowledgecenter/SSSA5P_12.7.0/ilog.odms.ide.help/refjavaopl/html/ilog/cplex/IloCplex.CplexStatus.html. Cplex actually has both a detailed (IloCplex.CplexStatus) and simple solver status (IloCplex.Status) available.
Looks good. Few minor comments. |
Additionally, some extra brackets have been added for clarity.
All basic changes implemented. Awaiting your response for the others. |
Any updates on this one? |
|
Fixes #22