lanl-ansi / Juniper.jl

A JuMP-based Nonlinear Integer Program Solver
https://lanl-ansi.github.io/Juniper.jl/stable/
MIT License
179 stars 22 forks source link

Restore randomnes after optimizing with Juniper. #191

Closed tweisser closed 4 years ago

tweisser commented 4 years ago

In order to get reproducible results, Juniper sets Random.seed! at some few places. This makes rand() basically useless, when used in a loop together with Juniper:

vals = zeros(10)
for i = 1:10
    m = some_model_generating_function(rand(); model = Model(Juniper.Optimizer()))
    optimize!(m)
    vals[i] = objective_value(m)
end

The entries of vals[2:10] are all the same. This is because when calling optimize! for the first time, Juniper sets the seed and hence, when calling rand() afterwards, it will always return the same number.

This PR resolves the problem by storing a random number before setting the seed and resetting the seed to this random number, before returning a solution.

Wikunia commented 4 years ago

Thanks for opening the issue. I'm wondering whether the best option would be to create an own random number generator like.

const JUNIPER_RNG = MersenneTwister(1);

and then call rand(JUNIPER_RNG, ...) that would make sure that it doesn't interfere with the outside system in any way. What do you think @tweisser & @ccoffrin

tweisser commented 4 years ago

Your suggestion is much cleaner than what I did. I pushed the changes.

Wikunia commented 4 years ago

Great! Does it still work as expected on your side?

tweisser commented 4 years ago

Yes, it does. I added a test in test/basic.jl.Maybe you have a better place for the test?

Wikunia commented 4 years ago

No I think that is an appropriate test case.

Wikunia commented 4 years ago

This looks good to me once the test cases run through. Which currently has some general problems with Ipopt if there are not magically fixed yet :smile:

tweisser commented 4 years ago

Sorry, no time to help with the Ipopt issue...

Wikunia commented 4 years ago

No worries thanks for this PR. I've just opened an issue #192 for that. Will work on it hopefully tmr. Then I can merge your PR and register a new version.

Thanks again!

ccoffrin commented 4 years ago

@Wikunia, thanks for the suggestion of a dedicated number generator. I was going to make the same one. :-)

Changes look good to me. There should be a way for the user to set the internal seed as well (to allow testing the impact of the seed on solver performance), but this can be filed as a feature issue outside of this PR.

odow commented 4 years ago

There should be a way for the user to set the internal seed as well

If I'm not mistaken, you should be able to go

Random.seed!(Juniper.JUNIPER_RNG, 1234)
Wikunia commented 4 years ago

Oh yeah that should work @odow that is very neat!

ccoffrin commented 4 years ago

Good point! We just need to document this constant JUNIPER_RNG.

We might also consider exposing it as a solver parameter as well. I think this is where most users are used to setting the seed of a solver.

codecov[bot] commented 4 years ago

Codecov Report

Merging #191 into master will decrease coverage by 1.11%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
- Coverage   92.57%   91.46%   -1.12%     
==========================================
  Files          22       22              
  Lines        2008     1862     -146     
==========================================
- Hits         1859     1703     -156     
- Misses        149      159      +10     
Impacted Files Coverage Δ
src/Juniper.jl 100.00% <ø> (ø)
src/BnBTree.jl 92.60% <100.00%> (-1.46%) :arrow_down:
src/MOI_wrapper/MOI_wrapper.jl 85.41% <100.00%> (+0.76%) :arrow_up:
src/fpump.jl 93.33% <100.00%> (+0.63%) :arrow_up:
src/util.jl 86.92% <100.00%> (-0.85%) :arrow_down:
src/MOI_wrapper/objective.jl 50.00% <0.00%> (-50.00%) :arrow_down:
src/MOI_wrapper/nlp.jl 33.33% <0.00%> (-33.34%) :arrow_down:
src/MOI_wrapper/constraints.jl 57.14% <0.00%> (-28.58%) :arrow_down:
src/solver.jl 64.58% <0.00%> (-14.43%) :arrow_down:
src/bb_integral_or_branch.jl 96.77% <0.00%> (-3.23%) :arrow_down:
... and 12 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 e2a7299...8043cca. Read the comment docs.

Wikunia commented 4 years ago

Don't get the problem with code coverage here. LGTM

ccoffrin commented 4 years ago

LGTM, I say go for it. Make sure to take a note in the change log.