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

MOI wrapper #149

Merged
merged 42 commits into from
Oct 12, 2020
Merged

MOI wrapper #149

merged 42 commits into from
Oct 12, 2020

Conversation

blegat
Copy link
Contributor

@blegat blegat commented Jul 20, 2020

The following branch already a branch that started an MOI port https://github.com/lanl-ansi/Alpine.jl/tree/moi-wrapper.
In addition this branch also brings several improvement to Alpine but it is still a WIP.
We thought with @tweisser that it could be possible to make an MOI port that was less ambitious to provide MOI support to Alpine before this other branch is finished.
So the goal of this PR is to try to make the minimal changes of Alpine while switching from MPB to MOI.
The behavior of Alpine before and after this PR should be as close as possible, further changes that are not strictly necessary for the port should come in a later PR.

@ccoffrin
Copy link
Member

OMG, thank you @blegat!

@ccoffrin
Copy link
Member

@tweisser!

@harshangrjn
Copy link
Collaborator

@blegat @tweisser Thank you!

@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #149 into master will decrease coverage by 0.76%.
The diff coverage is 75.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   75.99%   75.23%   -0.77%     
==========================================
  Files          15       16       +1     
  Lines        3183     3359     +176     
==========================================
+ Hits         2419     2527     +108     
- Misses        764      832      +68     
Impacted Files Coverage Δ
src/Alpine.jl 100.00% <ø> (ø)
src/heuristics.jl 36.23% <16.12%> (-0.70%) ⬇️
src/matrix_opt_interface.jl 28.00% <28.00%> (ø)
src/operators.jl 73.42% <43.33%> (-0.36%) ⬇️
src/utility.jl 48.87% <49.54%> (+0.96%) ⬆️
src/multi.jl 60.68% <62.50%> (-3.02%) ⬇️
src/presolve.jl 78.33% <66.66%> (-0.80%) ⬇️
src/algorithm.jl 76.19% <81.81%> (-0.35%) ⬇️
src/bounds.jl 82.64% <83.33%> (-3.65%) ⬇️
src/moi_function2expr.jl 86.20% <86.20%> (ø)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72814dd...f2735ff. Read the comment docs.

src/moi_function2expr.jl Outdated Show resolved Hide resolved
@blegat
Copy link
Contributor Author

blegat commented Jul 24, 2020

I suggest

function MOI.add_constraint(model::Optimizer, f::Union{MOI.ScalarAffineFunction, MOI.ScalarQuadraticFunction}, set)
    push!(model.constr_expr_orig, _moi_function_to_expr(func))
    push!(model.constraint_bounds_orig, MOI.NLPBoundsPair(_lower(set), _upper(set)))
    push!(m.constr_structure, f isa MOI.ScalarAffineFunction ? :generic_linear : :generic_nonlinear)
    return MOI.ConstraintIndex{typeof(f), typeof(set)}(length(model.constr_expr_orig))
end

src/solver.jl Outdated Show resolved Hide resolved
src/solver.jl Outdated Show resolved Hide resolved
@harshangrjn
Copy link
Collaborator

@blegat While disabling the failing tests, its probably good to keep track of the ones disabled so that we can fix them later. The reason is I had disabled a few tests too as the convergence checks can vary from version of MIP solvers. Will need to work on filtering out inconsistent tests.

@blegat
Copy link
Contributor Author

blegat commented Aug 12, 2020

I agree, I'm keeping track of the list of them and we post a list. I don't know yet if it's a regression from Pavito or Alpine though

@harshangrjn
Copy link
Collaborator

@blegat that's great!

@blegat blegat marked this pull request as ready for review August 13, 2020 07:43
@blegat
Copy link
Contributor Author

blegat commented Aug 13, 2020

The PR is ready to be merged, I have left some issues that might be good to close before releasing: #150, #151 and #152.

I'd suggest to first merge this PR and close these issues as separate PR so that people can already try this version while these issues are resolved and report or fix any issue they might encounter.
This approached worked well for jump-dev/Pavito.jl#21 as it allowed @tasseff to make two PRs: jump-dev/Pavito.jl#26, jump-dev/Pavito.jl#29 fixing some issues with the MOI wrapper.

@blegat
Copy link
Contributor Author

blegat commented Sep 1, 2020

Bump :)

@harshangrjn harshangrjn merged commit 2a673f8 into lanl-ansi:master Oct 12, 2020
@harshangrjn
Copy link
Collaborator

@blegat and @tweisser This has been merged and thanks a bunch for your help in this migration. There are a few checks in runtests.jl which need to be fixed, which I shall do it on a separate PR, in addition to other open issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants