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

remove 'presolve' hook #275

Merged
merged 1 commit into from
Sep 26, 2014
Merged

remove 'presolve' hook #275

merged 1 commit into from
Sep 26, 2014

Conversation

mlubin
Copy link
Member

@mlubin mlubin commented Sep 26, 2014

@joehuchette, if this isn't really needed, I'd prefer to remove this functionality in favor of buildInternalModel(); ...; solve(). How badly does this break things?
Ref discussion in 28c8a5d.

@joehuchette
Copy link
Contributor

As far as I know, it'll just break the example code I sent you today.

@joehuchette
Copy link
Contributor

Should remove the export as well (or else it's going to stick around for like a year until someone rediscovers it...)

@mlubin
Copy link
Member Author

mlubin commented Sep 26, 2014

Ah, didn't realize it was exported.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 4d83255 on remove_presolve into c7418d3 on master.

mlubin added a commit that referenced this pull request Sep 26, 2014
@mlubin mlubin merged commit 7eefb5d into master Sep 26, 2014
@mlubin mlubin deleted the remove_presolve branch September 26, 2014 22:14
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 4d83255 on remove_presolve into c7418d3 on master.

@joehuchette
Copy link
Contributor

@juan-pablo-vielma pointed out that this was being used in CPLEX.jl to implement branch/incumbent callbacks. Any opposition to restoring?

@mlubin
Copy link
Member Author

mlubin commented Jan 15, 2015

Could you use the buildInternalModel(); ...; solve() pattern instead?
You'll need to wrap solve with a new name, but that seems to be the standard approach currently for extensions.

@joehuchette
Copy link
Contributor

It seems kludgy to me to force the user to use something like solve_with_extra_callbacks(m::JuMP.Model) since this isn't really an extension, more like an extra feature that lives in one of the solver packages.

In other words, something like

solve(m::StochasticModel)

is kosher, but since we can't add a new method to dispatch on m::Model, you lose the generic-ness in user code.

@mlubin
Copy link
Member Author

mlubin commented Jan 15, 2015

Any code using incumbent or branching callbacks is already not generic. What about

solvecpx(m, branchcb = callback1, incumbentcb = callback2)

instead of

setBranchCallback(m, callback1)
setIncumbentCallback(m, callback2)
solve(m)

If we're planning on documenting the CPLEX callbacks and making it official that's one thing, but as it is now this would get things working without adding extra kludge to JuMP itself.
I do think we could use a way to plug into solve() for extensions, but a presolve hook isn't really sufficiently general.

@joehuchette
Copy link
Contributor

I mean it ideally would look generic---the fact that CPLEX is the only solver to support it, and where exactly the code lives should be immaterial to the end user. What would a more general solve hook look like? I can reshape the code if you have something in mind, but as it is right now this serves a real purpose, seems reasonably general and useful, and is a pretty tiny addition to the codebase.

@mlubin
Copy link
Member Author

mlubin commented Jan 15, 2015

Something like setSolveHook that gets called instead of the default solve if it's set, with a special keyword like solve(m, ignore_solve_hook=true) so that you could call JuMP's solve from inside of the solve hook.
Would this work for JuMPeR, @IainNZ?

@joehuchette
Copy link
Contributor

Sounds reasonable to me.

@IainNZ
Copy link
Collaborator

IainNZ commented Jan 15, 2015

Could we have a setHook with a solve or print option? That'd be the two awkward things with "RobustModel" resolved.

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

4 participants