Skip to content

Remove JuMP dependency#207

Merged
Wikunia merged 12 commits intolanl-ansi:masterfrom
blegat:bl/jump-moi
Feb 22, 2021
Merged

Remove JuMP dependency#207
Wikunia merged 12 commits intolanl-ansi:masterfrom
blegat:bl/jump-moi

Conversation

@blegat
Copy link
Collaborator

@blegat blegat commented Feb 11, 2021

This is an attempt to simplify the code and remove the JuMP dependency. The hope is to simply use a cache and copy_to to generalize Juniper to any constraint type.

@ccoffrin
Copy link
Member

Nice, I did not realize this was even possible!

@blegat
Copy link
Collaborator Author

blegat commented Feb 11, 2021

Another advantage for copy_to is that it will be faster than the current one:

MOI.add_constraint(backend, constr[1], constr[2])
which has type unstability issues

@codecov
Copy link

codecov bot commented Feb 14, 2021

Codecov Report

Merging #207 (0c3cdd0) into master (661f586) will decrease coverage by 1.20%.
The diff coverage is 79.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
- Coverage   92.19%   90.98%   -1.21%     
==========================================
  Files          22       20       -2     
  Lines        2012     1842     -170     
==========================================
- Hits         1855     1676     -179     
- Misses        157      166       +9     
Impacted Files Coverage Δ
src/Juniper.jl 100.00% <ø> (ø)
src/printing.jl 76.47% <ø> (-2.48%) ⬇️
src/types.jl 100.00% <ø> (ø)
src/util.jl 69.33% <56.60%> (-18.44%) ⬇️
src/MOI_wrapper/MOI_wrapper.jl 84.26% <78.57%> (+1.10%) ⬆️
src/BnBTree.jl 93.71% <93.33%> (+0.27%) ⬆️
src/fpump.jl 88.60% <93.33%> (-3.40%) ⬇️
src/filter.jl 96.55% <96.55%> (ø)
src/MOI_wrapper/results.jl 80.76% <100.00%> (ø)
src/bb_inits_and_defaults.jl 100.00% <100.00%> (ø)
... and 6 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 661f586...0c3cdd0. Read the comment docs.

@blegat blegat force-pushed the bl/jump-moi branch 2 times, most recently from 249e177 to d1acc6d Compare February 17, 2021 19:21
@blegat
Copy link
Collaborator Author

blegat commented Feb 18, 2021

Should be ready for review :)
With this PR, Juniper should now work for any constraint type (so it should for instance work for MISDP if the nlp solver supports PSD constraints) although it probably still needs some work to support other mixed-integer sets such as Indicator, SOS, Semiinteger, ... as currently, it will try to copy it to the nonlinear solver and it will result in an UnsupportedConstraint` error. We probably just need to filter out but I'll leave that for another PR.

Copy link
Member

@Wikunia Wikunia left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Overall it looks much cleaner.

Just some basic questions for now beside the one comment I've added:
Will MOI use bridges automatically for the MIP part in the feasibility pump if for example the solver doesn't support >= just <= ? That's more a general MOI question as you know that better than me 😉

It seems like we don't need the extra register for user defined functions anymore, right?
If that is actually the case first of all I'm amazed 🎉
Would be great if you can remove https://lanl-ansi.github.io/Juniper.jl/stable/options/#registered_functions::Union%7BNothing,Vector%7BRegisteredFunction%7D%7D-%5Bnothing%5D from the docs then as well.

@ccoffrin before we merge it would be in general nice to run the regression tests again to see if any problem fails or if there is speedup/slowdown.

@blegat
Copy link
Collaborator Author

blegat commented Feb 18, 2021

Will MOI use bridges automatically for the MIP part in the feasibility pump if for example the solver doesn't support >= just <= ?

Yes, we use MOI.instantiate(solver, with_bridge_type=Float64) which adds a bridge layer will all the bridges just like you have with JuMP.

It seems like we don't need the extra register for user defined functions anymore, right?

Yes :) will remove

@Wikunia Wikunia added the regression needed Needs regression testing before merge label Feb 18, 2021
@Wikunia Wikunia mentioned this pull request Feb 18, 2021
@ccoffrin
Copy link
Member

@Wikunia, sure thing I'll do the full regression.

@ccoffrin
Copy link
Member

While testing this I was wondering if JuMP can now be moved from deps to extras on the Project.toml?

@blegat
Copy link
Collaborator Author

blegat commented Feb 21, 2021

I was wondering if JuMP can now be moved from deps to extras on the Project.toml?

Yes, done :)

@Wikunia
Copy link
Member

Wikunia commented Feb 22, 2021

I have some great news @blegat & @ccoffrin

With one processor the MOI version is quite a bit better
juniper_moi

With four cores it is better earlier on and then they solve the same number of instances
juniper_moi-p4

For 8 cores it looks quite similar to the four cores one ;)

@ccoffrin
Copy link
Member

Nice! Shall we merge and release then?

Maybe we should tag the current master before merging?

@Wikunia
Copy link
Member

Wikunia commented Feb 22, 2021

You mean current master as v0.6.6 and this as v0.7.0 ?
The current one doesn't have any new changes beside github action, right? So it's not strictly necessary I assume?

@ccoffrin
Copy link
Member

You are correct. Nothing new on master, no v0.6.6 release is required.

@Wikunia Wikunia merged commit 179169c into lanl-ansi:master Feb 22, 2021
@Wikunia Wikunia mentioned this pull request Feb 22, 2021
@blegat blegat mentioned this pull request Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

regression needed Needs regression testing before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants