Skip to content
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

fix empty optimization result + test of infeasible ip restricted master #173

Merged
merged 4 commits into from
Aug 28, 2019

Conversation

guimarqu
Copy link
Contributor

@guimarqu guimarqu commented Aug 26, 2019

Fix #167
Raise and fix #174

@guimarqu guimarqu added the bug Something isn't working label Aug 26, 2019
@codecov-io
Copy link

codecov-io commented Aug 26, 2019

Codecov Report

Merging #173 into master will increase coverage by 0.18%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
+ Coverage   76.87%   77.06%   +0.18%     
==========================================
  Files          38       38              
  Lines        2171     2193      +22     
==========================================
+ Hits         1669     1690      +21     
- Misses        502      503       +1
Impacted Files Coverage Δ
src/optimizationresults.jl 89.58% <100%> (+0.45%) ⬆️
src/optimizerwrappers.jl 82.27% <100%> (+4.94%) ⬆️
src/MOIinterface.jl 85.16% <100%> (ø) ⬆️
src/algorithms/reformulationsolver.jl 95.08% <100%> (+1.48%) ⬆️
src/algorithms/masteripheur.jl 100% <100%> (ø) ⬆️
src/incumbents.jl 77.08% <33.33%> (-2.92%) ⬇️
src/formulation.jl 73.27% <46.66%> (-0.12%) ⬇️
src/MOIwrapper.jl 86.95% <50%> (ø) ⬆️
src/node.jl 85% <0%> (-1.67%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94db792...0e69800. Read the comment docs.

@guimarqu
Copy link
Contributor Author

@vitornesello GLPK throws an exception when you give it a model without variables.
Can we check that there is no variable in the model, print a warning, and then return infeasible ?

@guimarqu guimarqu requested a review from vitornesello August 28, 2019 00:42
@guimarqu guimarqu marked this pull request as ready for review August 28, 2019 00:43
@@ -50,8 +50,8 @@ getprimalsols(res::OptimizationResult) = res.primal_sols
getdualsols(res::OptimizationResult) = res.dual_sols
nbprimalsols(res::OptimizationResult) = length(res.primal_sols)
nbdualsols(res::OptimizationResult) = length(res.dual_sols)
getbestprimalsol(res::OptimizationResult) = res.primal_sols[1]
getbestdualsol(res::OptimizationResult) = res.dual_sols[1]
getbestprimalsol(res::OptimizationResult) = get(res.primal_sols, 1, nothing)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should leave it to the user to ask if there is a solution available in case he is not sure.
We could add a function issolavailable
Anyway after calling getbestsol he will need to check if the return value was nothing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add nbprimalsol() and nbdualsol()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are already there..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what is the best behavior? In any case, the user should assure himself that there is at least one solution available, it he uses directly the result of the new getbestsol he might also get in trouble

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine for now if it returns nothing. I added new methods to handle this case e.g.
set_lp_primal_sol!(inc::Incumbents, ::Nothing) = false. If the user works on the solution, he will have to test that solution != nothing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means we will have to add methods for functions that expect a solution all over the place.

problem, x, dec = CLD.GeneralizedAssignment.model(data, coluna)

JuMP.optimize!(problem)
@test JuMP.objective_value(problem) == Inf
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also check the feasibility status?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It returns INFEASIBLE. I think it should be NODE_LIMIT. We should do that in another PR and tests all possible termination statuses.

@guimarqu guimarqu requested a review from vitornesello August 28, 2019 17:59
@guimarqu guimarqu merged commit 54779ad into master Aug 28, 2019
@guimarqu guimarqu deleted the fix_optres branch August 28, 2019 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants