Skip to content

Update to MOI v0.9 #66

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Oct 30, 2019
Merged

Update to MOI v0.9 #66

merged 17 commits into from
Oct 30, 2019

Conversation

mtanneau
Copy link
Contributor

@mtanneau mtanneau commented Oct 8, 2019 •
edited
Loading

As of now, the MOI re-write is partially working.
Still some work to do but I think most heavy-lifting is done 😅

Existential questions

  • What is the role of Clp.ClpSolve? So far, I have not integrated that in the MOI wrapper.
    Sounds like I should, but I don't know how it integrates with the rest.
  • It seems like Clp negates the constant term in the objective. Passing -f.constant to set_objective_offset seems to have fixed problems. Not sure whether it's the actual Clp convetion, or just me who did something wrong.

Things that remain to be implemented

  • Currently not supported MOI attributes:
    • TimeLimitSec: Clp behaves weird, cf maximum_seconds returns weird results #65
      [EDIT: not supported until Clp's behaviour is fixed]
    • SolveTime: I did not find the corresponding function is Clp
      Possible workaround: use an @elapsed when calling the Clp-level solve, and store that value at the wrapper level.
      [EDIT: not supported]
    • DualObjectiveValue: I'm not sure whether Clp has a corresponding function.
      I thought it would be Clp.dual_bound, but that essentially returned + infinity when I tried it
      [EDIT: not supported]
    • NumberOfThreads: I don't know whether Clp is always single-threaded (in which case we could return 1 if queried, but prevent the user from setting it), or if this should just not be supported. Did not find a thread-related function in the C interface.
      [EDIT: not supported]
  • Quadratic objective: Clp supports it but I have not included it (yet)
    [EDIT: left for future work]
  • Dupplicate terms in ScalarAffineFunction constraints. I do not check that at the wrapper level, rather I'm assuming that the cache automatically gives functions in canonical form
  • Print levels: although MOI.Silent is set to false at the beginning of the tests, and its value is kept intact when MOI.empty! is called, Clp still prints during the tests. Help welcome.
  • Keep track of parameters/options at the wrapper level. I'm not sure I'm 100% sure of what the convention w.r.t parameters and empty!. Should parameters also be reset to their default values?

Tests that still fail

Non-exhaustive list. See which tests I removed here

  • solve_result_index: I removed it from the tests, it calls MOI.get(m, MOI.ObjectiveValue(2)).
    Not sure how this should be handled here.
  • Some tests throw errors while using VectorAffineFunction or VectorOfVariables. I though these should be handled by the bridge. See for instance error message below
delete_nonnegative_variables: Test Failed at /home/mathieu/.julia/packages/MathOptInterface/iPJzh/src/Test/UnitTests/variables.jl:114
  Expression: !(MOI.is_valid(model, cv))
Stacktrace:
 [1] delete_nonnegative_variables(::MathOptInterface.Bridges.LazyBridgeOptimizer{MathOptInterface.Utilities.CachingOptimizer{Clp.Optimizer,MathOptInterface.Utilities.UniversalFallback{MathOptInterface.Utilities.Model{Float64}}}}, ::MathOptInterface.Test.TestConfig{Float64}) at /home/mathieu/.julia/packages/MathOptInterface/iPJzh/src/Test/UnitTests/variables.jl:114
 [2] macro expansion at /home/mathieu/.julia/packages/MathOptInterface/iPJzh/src/Test/config.jl:47 [inlined]
 [3] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:1083 [inlined]
 [4] unittest(::MathOptInterface.Bridges.LazyBridgeOptimizer{MathOptInterface.Utilities.CachingOptimizer{Clp.Optimizer,MathOptInterface.Utilities.UniversalFallback{MathOptInterface.Utilities.Model{Float64}}}}, ::MathOptInterface.Test.TestConfig{Float64}, ::Array{String,1}) at /home/mathieu/.julia/packages/MathOptInterface/iPJzh/src/Test/config.jl:47
  • linear8a: some of the duals appear to be wrong it's likely something on my end.
    [EDIT: linear8a and linear12 were excluded from the tests]

@mtanneau
Partially working update to MOI v0.9
3494388 
* Drop LQOI and bump MOI version in dependencies
* Re-write the MOI wrapper based on MOI.copy_to
@odow
Copy link
Member

odow commented Oct 9, 2019

What is the role of Clp.ClpSolve? So far, I have not integrated that in the MOI wrapper.
Sounds like I should, but I don't know how it integrates with the rest.

Not sure. I'd just follow the MPB wrapper.

It seems like Clp negates the constant term in the objective. Passing -f.constant to set_objective_offset seems to have fixed problems. Not sure whether it's the actual Clp convetion, or just me who did something wrong.

Is this a min/max problem?

Quadratic objective: Clp supports it but I have not included it (yet)

No rush to include. Can be latest PR. But not sure how well it's tested.

Dupplicate terms in ScalarAffineFunction constraints. I do not check that at the wrapper level, rather I'm assuming that the cache automatically gives functions in canonical form

You should not assume that the passed function is canonicalized; call MOI.Utilities.canonical.

Print levels: although MOI.Silent is set to false at the beginning of the tests, and its value is kept intact when MOI.empty! is called, Clp still prints during the tests. Help welcome.

Keep track of parameters/options at the wrapper level. I'm not sure I'm 100% sure of what the convention w.r.t parameters and empty!. Should parameters also be reset to their default values?

I think we've kept the parameters.

solve_result_index: I removed it from the tests, it calls MOI.get(m, MOI.ObjectiveValue(2)).

You should throw MOI.ResultIndexBoundsError or call MOI.check_result_index_bounds(model, attr)

Some tests throw errors while using VectorAffineFunction or VectorOfVariables. I though these should be handled by the bridge. See for instance error message below

Bug in MOI. Wait for new tag.

linear8a: some of the duals appear to be wrong it's likely something on my end.

Not sure.

blegat
blegat reviewed Oct 9, 2019
View reviewed changes
blegat
blegat reviewed Oct 9, 2019
View reviewed changes
blegat
blegat reviewed Oct 9, 2019
View reviewed changes
blegat
blegat reviewed Oct 9, 2019
View reviewed changes
blegat
blegat reviewed Oct 9, 2019
View reviewed changes
blegat
blegat reviewed Oct 9, 2019
View reviewed changes
blegat
blegat reviewed Oct 9, 2019
View reviewed changes
blegat
blegat reviewed Oct 9, 2019
View reviewed changes
blegat
blegat reviewed Oct 9, 2019
View reviewed changes
blegat
blegat reviewed Oct 9, 2019
View reviewed changes
blegat
blegat reviewed Oct 9, 2019
View reviewed changes
frapac
frapac reviewed Oct 9, 2019
View reviewed changes
Copy link

@frapac frapac left a comment

Choose a reason for hiding this comment

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

Nice job!

@mtanneau
Address minor comments
ddcf387 
* Remove support for SingleVariable objective (will be handled by bridge)
* Throw MOI exceptions instead of general errors
* Use more compact notation when applicable
* Canonicalize linear constraints before passing them to Clp
* Bump MOI version requirement to MOIv0.9.4
* Update silent parameter
@mtanneau
Free current Clp model in MOI.empty!
96c6380
@mtanneau
Support MOI.RawStatusString
caacbb3
@mtanneau
Fix handling of dual solutions
212e8e7
@mtanneau
Copy link
Contributor Author

What is the role of Clp.ClpSolve? So far, I have not integrated that in the MOI wrapper.
Sounds like I should, but I don't know how it integrates with the rest.

Not sure. I'd just follow the MPB wrapper.

This is next on the TODO list.

It seems like Clp negates the constant term in the objective. Passing -f.constant to set_objective_offset seems to have fixed problems. Not sure whether it's the actual Clp convetion, or just me who did something wrong.

Is this a min/max problem?

Doesn't appear so. I tried with baby examples, and the objective offset seems to be always negated.
All related tests are passing, so I guess it's just Clp's convention.

Quadratic objective: Clp supports it but I have not included it (yet)

No rush to include. Can be latest PR. But not sure how well it's tested.

It will be left for future work then

Keep track of parameters/options at the wrapper level. I'm not sure I'm 100% sure of what the convention w.r.t parameters and empty!. Should parameters also be reset to their default values?

I think we've kept the parameters.

That will be done together with the ClpSolve part. Coming soon.

linear8a: some of the duals appear to be wrong it's likely something on my end.

Not sure.

These are related to infeasibility rays taking wrong values. Also causes failures on linear12.

@blegat blegat mentioned this pull request Oct 13, 2019
Closed
20 tasks
@mtanneau
Fix handling of solver parameters
25a1ef7 
* Add ClpSolve options to Clp.Optimizer
* Copy solver parameters when calling MOI.empty!
@mtanneau
Copy link
Contributor Author

Solver parameters / solve option are now tracked automatically, and they are not discarded during an MOI.empty!.
The only (raw) parameter that is currently not supported is maximumIterations, see #67 and #69.
Once a fix is implemented, it's just one line to un-comment.

Currently in the pipeline: implement these check_result_index_bounds where needed.

The last two things left to do are 1) whether MOI.DualObjectiveValue should be supported or not, and 2) find out why tests linear12 and linear8a are failing. It has something to do with querying the dual certificates for an infeasible problem.

@blegat
Copy link
Member

blegat commented Oct 14, 2019

It has something to do with querying the dual certificates for an infeasible problem.

Clp is not good with that, see #18 and #20. I would not be surprised if it's a Clp bug rather than a bug in the MOI wrapper

@mtanneau
Copy link
Contributor Author

I removed the tests that query infeasibility certificates, and de-activated DualObjectiveValue in the test config.

The latter needs MOI0.9.6 to work. Once the new version is tagged all tests should pass.

If that's satisfactory then the PR can be merged then.

@mtanneau mtanneau mentioned this pull request Oct 27, 2019
Closed
@mtanneau mtanneau changed the title DNMY: Update to MOI v0.9 Update to MOI v0.9 Oct 30, 2019
blegat
blegat reviewed Oct 30, 2019
View reviewed changes
@blegat blegat merged commit e1d46fd into jump-dev:master Oct 30, 2019
This was referenced Jan 6, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blegat blegat blegat approved these changes

@matbesancon matbesancon matbesancon left review comments

@frapac frapac frapac left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mtanneau @odow @blegat @matbesancon @frapac @juan-pablo-vielma