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

Permit info callbacks in states other than MIPInfo #814

Merged
merged 1 commit into from
Sep 12, 2016
Merged

Permit info callbacks in states other than MIPInfo #814

merged 1 commit into from
Sep 12, 2016

Conversation

yeesian
Copy link
Contributor

@yeesian yeesian commented Jul 31, 2016

The use-case is to allow for the extraction of intermediate solutions with information callbacks.

ref: JuliaOpt/MathProgBase.jl#115, jump-dev/Gurobi.jl#59, jump-dev/CPLEX.jl#80, JuliaOpt/GLPKMathProgInterface.jl#32

push!(objs, MathProgBase.cbgetobj(cb))
push!(bestbounds, MathProgBase.cbgetbestbound(cb))
if MathProgBase.cbgetstate(cb) == :Other
push!(nodes, MathProgBase.cbgetexplorednodes(cb))
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean? It's invalid to call MathProgBase.cbgetexplorednodes if state is not :Other? How are people supposed to know what is valid to call inside an info callback?

Copy link
Contributor Author

@yeesian yeesian Jul 31, 2016

Choose a reason for hiding this comment

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

Previously, the callback will only fire when the state is :MIPInfo (which is supposed to be :Other in MPB-land). It's to let current tests pass, since Gurobi.jl does not allow for MPB.cbgetobj to work when the state is :MIPSol.

@codecov-io
Copy link

codecov-io commented Jul 31, 2016

Current coverage is 84.60% (diff: 16.66%)

Merging #814 into master will decrease coverage by 0.23%

@@             master       #814   diff @@
==========================================
  Files            18         18          
  Lines          4327       4294    -33   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           3671       3633    -38   
- Misses          656        661     +5   
  Partials          0          0          

Powered by Codecov. Last update 6fba39a...cbe0fe2

@mlubin
Copy link
Member

mlubin commented Aug 1, 2016

I don't really get this :Other thing. Can we update the MPB docs to make info callbacks more coherent?

@yeesian
Copy link
Contributor Author

yeesian commented Aug 1, 2016

From my reading (primarily of the master callback functions),

Which is how I imagine MPB came to have the current situation of :MIPNode, :MIPSol or :Other.

The PR at jump-dev/Gurobi.jl#59 is to make the state information available in the info callback so that you can extract info at each MIPSol. In the process, :MIPInfo has been changed to :Other (for Gurobi.jl) in accordance with MPB's guidelines.

I don't pretend this fixes the situation for the other solvers, or that all other state information should be coerced into :Other. But that requires a larger investigation into the different kinds of states and callbacks to support at MPB-level, which is beyond the scope of this PR.

@mlubin
Copy link
Member

mlubin commented Aug 1, 2016

This PR together with jump-dev/Gurobi.jl#59 breaks user code using Gurobi with info callbacks, I assume. If we're going to do this, it should be an obvious improvement and not unnecessarily introduce confusing things like :Other. So it's certainly within the scope of this PR to make info callbacks more coherent in general, otherwise we'll have to break user code again in the future when this happens.

@mlubin
Copy link
Member

mlubin commented Aug 1, 2016

I'm ok with removing the @assert state == :MIPInfo if that helps you out, but not with the changes to the tests.

@yeesian
Copy link
Contributor Author

yeesian commented Aug 1, 2016

To summarise the issues here:

  1. Opening infocallbacks to fire on all states (instead of an unstated/limited subset; see point 2) changes their current behavior, as you've intuited/pointed-out. This might be a breaking change if users relied on unstated/implicit assumptions of where the infocallback function is supposed to fire.
  2. Currently infocallback state info is munged into :Other, so it only fires when gurobi is in state CB_MIP. If we remove @assert state == :MIPInfo, the tests will need to be changed (like in this PR) to preserve meaning.

We can proceed in a number of ways. Two possibilities that comes to mind:

  • make it the responsibility of the user to check which state the infocallback is in, to determine which methods are available, the way it is done in the infocallbacks test in this PR. This ought to have been the case for defensive users in the current setup (see point 1 above).
  • provide an additional argument to the infocallback function, e.g. infocallback(m, mycb, [:MIPSol]), to specify the states in which the infocallback function will fire. That way, the current infocallback(m, mycb) can be aliased to infocallback(m, mycb, [:Other]) without breaking anything.

Some other auxiliary comments:

  • Currently Gurobi.jl breaks on cbgetobj() in state == :MIPSol (which I presume will get resolved in time), which caused point 1 above to become visible in the tests.
  • Perhaps it is okay to maintain the three states :MIPNode, :MIPSol, and :Other in MPB the way it is done now. One way of not losing information is to introduce a secondary/solver-specific state, so that all the other info isn't munged into :Other (c.f. point 2). This can be introduced in a lazy/iterative way in subsequent PRs.

@mlubin
Copy link
Member

mlubin commented Aug 1, 2016

Could we start out by thinking about what people might want to do with info callbacks and how we can make it possible and easy to do so? Checking state == :Other before calling accessor functions is just terribly unintuitive and doesn't necessarily make sense for all solvers.

@yeesian
Copy link
Contributor Author

yeesian commented Aug 1, 2016

Could we start out by thinking about what people might want to do with info callbacks and how we can make it possible and easy to do so?

I'll personally prefer start with a couple of documented examples of "here's how you can do X (with info callbacks)" that interested users can copy+paste. And maybe generalise only after we elicit a couple of use-cases, rather than upfront trying to figure it all out.

The anxiety over if MathProgBase.cbgetstate(cb) == :Other should be directed towards

and not with this PR (to permit info callbacks to fire in other states of the B&B tree).


Checking state == :Other before calling accessor functions is just terribly unintuitive and doesn't necessarily make sense for all solvers.

Having a generic fallback :Other would allow for users to have more customized callbacks (depending on their knowledge of their particular solver; and users can choose to ignore it if they prefer). I happen to see it as a net plus, rather than a minus, in terms of usability.

The check of MathProgBase.cbgetstate(cb) == :Other starts to feel less "special"/discomfiting when there are infocallbacks that keep track of different things at different states. For example, one thing I have found myself needing (that the current setup of infocallbacks don't allow for) is keeping track of the intermediate solutions in the B&B tree.

Here's how that can be done (with this PR):

function myinfo(cb)
    entered[1] = true
    if MathProgBase.cbgetstate(cb) == :Other
        push!(nodes,      MathProgBase.cbgetexplorednodes(cb))
        push!(objs,       MathProgBase.cbgetobj(cb))
        push!(bestbounds, MathProgBase.cbgetbestbound(cb))
    end
    if MathProgBase.cbgetstate(cb) == :MIPSol
        # Update all variable values so that `getvalue` works
        MathProgBase.cbgetmipsolution(cb, mod.colVal)
        println(JuMP.getvalue(x[1:15])) # print just 15 values to not overwhelm the example screen
    end
end

Giving something like

[callback] Test informational callback
  > With solver Gurobi.GurobiSolver
[1.0,1.0,1.0,1.0,1.0,1.0,1.0,1.0,0.0,1.0,0.0,0.0,0.0,0.0,0.0]
[0.0,0.0,0.0,0.0,0.0,0.0,0.0,0.0,0.0,0.0,0.0,0.0,0.0,0.0,0.0]
[-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0]
[-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0]
[-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0,-0.0]
5 facts verified.

Alternatively, I can store them all into an array, rather than printing them out. And again, I'll like to reiterate that the need to check for :Other is due to the issue with cbgetobj() in Gurobi.jl, and is otherwise not required.

@mlubin
Copy link
Member

mlubin commented Aug 1, 2016

How about being clear about when we want the info callback to be called? Something like

addinfocallback(mod, myinfo, when = :Feasible)   # MIPSOL in Gurobi
addinfocallback(mod, myinfo, when = :Fractional) # MIPNODE
addinfocallback(mod, myinfo, when = :BnBNode)    # MIP
addinfocallback(mod, myinfo, when = :Iteration)  # For barrier/simplex iterations, etc..

For :Feasible and :Fractional, JuMP can load the solution into the variables as it does for the lazy and cut callbacks.

@yeesian
Copy link
Contributor Author

yeesian commented Aug 1, 2016

My primary consideration will be to allow for solver-specific state/information, before we settle on a set of solver-independent descriptions. And I don't yet see that with the proposal above, unless we are to make the when clauses solver-specific (like we do with solver-creation options).

Update: I just realized the callback data has the raw/original information, so I was worrying for nothing.

How about being clear about when we want the info callback to be called?

Sure.

@yeesian
Copy link
Contributor Author

yeesian commented Aug 2, 2016

Thinking about it more, I remain unconvinced that

addinfocallback(mod, myinfo1, when = :Feasible)   # MIPSOL in Gurobi
addinfocallback(mod, myinfo2, when = :Fractional) # MIPNODE
addinfocallback(mod, myinfo3, when = :BnBNode)    # MIP
addinfocallback(mod, myinfo4, when = :Iteration)  # For barrier/simplex iterations, etc..

is any better than

function myinfo(cbdata)
    cbdata.state == :Feasible && myinfo1(cbdata)
    cbdata.state == :Fractional && myinfo2(cbdata)
    cbdata.state == :BnBNode && myinfo3(cbdata)
    cbdata.state == :Iteration && myinfo4(cbdata)
end    

addinfocallback(mod, myinfo)

or equivalently

addinfocallback(mod, cbdata -> begin
    cbdata.state == :Feasible && myinfo1(cbdata)
    cbdata.state == :Fractional && myinfo2(cbdata)
    cbdata.state == :BnBNode && myinfo3(cbdata)
    cbdata.state == :Iteration && myinfo4(cbdata)
end)

Both require the user to think about when the infocallback should be fired, and the latter results in a lot less work for the master callback function to keep track of all possible infocallback functions (myinfo1 to myinfo4) and dispatch accordingly. It's what all the other MPB callback functions have to do anyway, so I don't see why the infocallbacks should get special treatment.

@@ -38,9 +39,9 @@ function addheuristiccallback(m::Model, f::Function)
m.internalModelLoaded = false
push!(m.callbacks, HeuristicCallback(f))
end
function addinfocallback(m::Model, f::Function)
function addinfocallback(m::Model, f::Function, when::Symbol)
Copy link
Member

Choose a reason for hiding this comment

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

Keyword argument?

@yeesian yeesian changed the title Permit info callbacks in states other than MIPInfo [WIP] Permit info callbacks in states other than MIPInfo Aug 20, 2016
@yeesian
Copy link
Contributor Author

yeesian commented Aug 20, 2016

This PR is ready for review, after JuliaOpt/GLPKMathProgInterface.jl#32 is merged.

Edit: see latest comment

@yeesian yeesian changed the title [WIP] Permit info callbacks in states other than MIPInfo Permit info callbacks in states other than MIPInfo Aug 20, 2016
@joehuchette
Copy link
Contributor

I think I'm alright with this change, but throwing the error in addinfocallback if the where kwarg is omitted seems too breaking. Can we make it a deprecation warning instead?

@@ -41,7 +41,15 @@ function addheuristiccallback(m::Model, f::Function)
end
function addinfocallback(m::Model, f::Function; when::Symbol = :Undefined)
if when == :Undefined
error("Info Callbacks without a `when` clause are unsupported.")
when = :Intermediate
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't really matter, but I think you could do something like:

function _not_specified()
    warn("""Info Callbacks without a when clause will currently default
              to fire only in the :Intermediate state to preserve its behavior
              with previous versions of JuMP.

             This behavior will be deprecated in subsequent versions of JuMP, so
             please rewrite all invocations of addinfocallbacks(m, f) to
             addinfocallbacks(m, f, when = :Intermediate) instead.
             """)
    :Intermediate
end

function addinfocallback(m::Model, f::Function; when::Symbol = _not_specified())
...

to avoid the corner case where I might pass in when=:Undefined.

@joehuchette
Copy link
Contributor

What's the status here? Any objections @mlubin? If we're good to go, would you mind squashing @yeesian?

@mlubin
Copy link
Member

mlubin commented Aug 30, 2016

No objections, just note that we have to tag GLPKMathProgInterface, CPLEX, Gurobi, and JuMP all at the same time. JuMP release is currently broken against CPLEX master because of this change.

@joehuchette
Copy link
Contributor

Is this breaking enough that we can't backport to the release branch?

@mlubin
Copy link
Member

mlubin commented Aug 30, 2016

It's fine to tag master+this as 0.14.1.

On Aug 30, 2016 11:51, "Joey Huchette" notifications@github.com wrote:

Is this breaking enough that we can't backport to the release branch?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#814 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABp0M2aUx4haXzz9Ef_ld8j8qvwLEbWOks5qlG2ZgaJpZM4JZEwC
.

@joehuchette
Copy link
Contributor

Restarted Travis, will merge if passing.

@mlubin
Copy link
Member

mlubin commented Sep 7, 2016

If this passes before GLPK is tagged then something is broken

@chriscoey
Copy link
Contributor

This would be really useful for Pajarito. Can I help? Would be very nice to have this feature available ASAP.

@joehuchette
Copy link
Contributor

I can tag everything tomorrow

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.

None yet

5 participants