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 method to MOI.add_constrained_variable #386

Merged
merged 5 commits into from
Jan 8, 2021
Merged

Conversation

joehuchette
Copy link
Contributor

While doing some benchmarking, I noticed that calling MOI.add_constrained_variable(::Gurobi.Optimizer, set::Gurobi._SCALAR_SET) was spending roughly the same amount of time in GRBaddvar as it was in setting the bound attributes. This seemed a bit wasteful, so I added a new method that lives here that sets the bounds directly in GRBaddvar.

I'm a bit uncertain how to update lower_bound_if_bounded and upper_bound_if_bounded. The comments for _VariableInfo suggest that these fields should be NaN if and only if a variable has no bound constraints on it. That will never be the case in this method, so therefore I should set both bound values, even to +/-Inf for one-sided bounds. However, after looking through the rest of the file I'm not certain that this is the behavior actually implemented in, e.g.,

MOI.set(::Optimizer, ::MOI.ConstraintSet, ::MOI.ConstraintIndex{MOI.SingleVariable, S}, s::S) where {S <: Gurobi._SCALAR_SETS}

So, guidance would be appreciated on what I should do there.

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.

The whole _bound_if_bounded thing was a hack to work around SecondOrderCones, because when adding a SOC, we might change the bound of the epigraph variable to 0, so it's not sufficient to just query Gurobi's internal value of the bound.

A better way, going forward, would be to add an additional linear constraint that t >= 0, instead of modifying the bounds, but I don't have the time to implement.

I this method tested/used by JuMP?

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

Not by JuMP, I'm calling it directly in Cerberus. I'm not sure if this is being tested in MOI.Tests. Should I add a unit test in this repo?

@joehuchette
Copy link
Contributor Author

Screen Shot 2021-01-06 at 9 06 00 AM

@odow
Copy link
Member

odow commented Jan 6, 2021

A unit test would be good, if you have time.

@blegat
Copy link
Member

blegat commented Jan 7, 2021

Maybe also add a comment explaining why Gurobi.jl defines this method (as you explain in the first comment of thi PR)

@joehuchette
Copy link
Contributor Author

Screen Shot 2021-01-07 at 9 22 23 AM

@odow odow merged commit f6235a2 into master Jan 8, 2021
@odow odow deleted the add_constrained_variable branch January 8, 2021 07:09
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.

3 participants