Skip to content

Conversation

@odow
Copy link
Member

@odow odow commented Mar 31, 2015

Exposes MathProgBase.getsolvetime and checks if implemented by solver. #405

@mlubin
Copy link
Member

mlubin commented Mar 31, 2015

LGTM. The only question is if we should follow the MPB naming convention here, given that we decided to do so for numvar etc (#356). I'd lean towards extending the MPB method instead of defining a new name if it just accesses or imitates a property of the MPB solver.

src/solvers.jl Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think it's more Julia-like (for type stability reasons) to throw an exception when the value isn't available instead of returning nothing.

@mlubin
Copy link
Member

mlubin commented Mar 31, 2015

@IainNZ @joehuchette, comments on the naming issue?

@IainNZ
Copy link
Collaborator

IainNZ commented Mar 31, 2015

I'm leaning towards MathProgBase version, for now.

@joehuchette
Copy link
Contributor

I think I agree with Iain in theory, except for the fact that the current way to call it is so verbose with getInternalSolver.

@mlubin
Copy link
Member

mlubin commented Mar 31, 2015

@joehuchette, this wrapper lets you call it without getInternalSolver

@joehuchette
Copy link
Contributor

Understood, I'm thinking that I would just prefer a shorter alias for getInternalSolver

@mlubin
Copy link
Member

mlubin commented Mar 31, 2015

@joehuchette, the wrapper method lets you give a nice error message when the solver doesn't implement getsolvetime, which is pretty useful.

@mlubin
Copy link
Member

mlubin commented Apr 2, 2015

@odow, we can merge this as is. Could you squash the commits?

@odow
Copy link
Member Author

odow commented Apr 2, 2015

@mlubin squashed.

mlubin added a commit that referenced this pull request Apr 3, 2015
implement getsolvetime. issue #405
@mlubin mlubin merged commit 29615f0 into jump-dev:master Apr 3, 2015
@mlubin
Copy link
Member

mlubin commented Apr 3, 2015

Thanks!

@odow odow deleted the solvetime branch April 3, 2015 01:21
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.

4 participants