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

Add fake termination status to Gurobi (so it become compliant with MOI.MEMORY_LIMIT) #398

Merged
merged 4 commits into from
Mar 3, 2021

Conversation

henriquebecker91
Copy link
Contributor

This PR intends to solve #397. The PR creates a generic mechanism that allows Gurobi Error Codes to interpreted as termination status by the Gurobi wrapper. For now, it is used only for transforming OUT_OF_MEMORY in MOI.MEMORY_LIMIT, but new conversions only need to add a new element to a global internal Dict. The mechanism consist in one extra field for Gurobi.Optimizer (called fake_termination_status), a new internal function _check_ret_GRBoptimize, a new internal global _FAKE_TERMINATION_STATUS, and changes to _raw_status, Optimizer (constructor), is_empty, empty!, and optimize!.

The code is well commented. I did not add tests. If tests are deemed necessary, then I would like that someone more familiar with exception testing inside Gurobi.jl gave me some pointers.

The current code give some errors:

2021-03-03-15-55-31-1920x1080.png

but they are the same obtained in the master branch:

2021-03-03-15-50-47-1920x1080.png

and probably should be fixed in other PR:

2021-03-03-15-55-01-1920x1080.png

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Nice. Are there any other return codes we should catch?

# to Gurobi "Status" and instead we return the appropriate
# `MOI.TerminationStatus`. Just before calling `GRBoptimize` we change
# this to zero again.
fake_termination_status::Cint
Copy link
Member

Choose a reason for hiding this comment

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

ret_GRBoptimize::Cint?

src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member

odow commented Mar 3, 2021

Are you on a new release of Gurobi? Good to see they fixed it.

@henriquebecker91
Copy link
Contributor Author

Are you on a new release of Gurobi? Good to see they fixed it.

Sorry, fixed what exactly? The Farkas dual? I am using Gurobi 9.1.1.

@odow
Copy link
Member

odow commented Mar 3, 2021

There was a bug in Gurobi. But they fixed it which is why you now see "Got correct result"

@henriquebecker91
Copy link
Contributor Author

About the changes, I have no problem with the purely stylistic ones, and will probably make a new commit soon to solve them. However, some of the changes of names and how conditionals are done are pointing could be looked at with a little more attention.

fake_termination_status -> ret_GRBoptimize::Cint

model.retGRBoptimize = GRBoptimize(model)
_check_ret_GRBoptimize(model)

_FAKE_TERMINATION_STATUS -> _ERROR_CODES

Its seems to me that, instead of making this field only for the error codes that may be interpreted as TerminationStatuses, the idea is always saving the error code of the GRBoptimize, and then, if the termination status is queried, we look at the Dict. This alone is ok, even more elegant than what I have initially thought (I mean, probably haskey may end up called multiple times but then the overhead is minimal). However, I think in this case the Dict really should not be called _ERROR_CODES that gives the impression that all Gurobi _ERROR_CODES are stored there. Maybe _ERROR_TO_STATUS instead? It is shorter and not so misleading.

@odow
Copy link
Member

odow commented Mar 3, 2021

Maybe _ERROR_TO_STATUS instead?

Sure.

@henriquebecker91
Copy link
Contributor Author

Made the changes, the tests continue to pass:

2021-03-03-18-17-16-1920x1080.png

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Looks good.

src/MOI_wrapper.jl Outdated Show resolved Hide resolved
@henriquebecker91
Copy link
Contributor Author

Made the change, tests continue the same:

2021-03-03-19-05-56-1920x1080.png

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants