From 551d1202d2d16f05d402b8d17043dac0fd55807c Mon Sep 17 00:00:00 2001
From: Fredrik Bagge Carlson <baggepinnen@gmail.com>
Date: Sun, 14 Mar 2021 15:04:18 +0100
Subject: [PATCH] fix parallel history bug

---
 src/Hyperopt.jl  | 14 +++++++++-----
 test/runtests.jl |  4 ++++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/Hyperopt.jl b/src/Hyperopt.jl
index 5011c5e..c96212c 100644
--- a/src/Hyperopt.jl
+++ b/src/Hyperopt.jl
@@ -112,7 +112,7 @@ function create_ho(params, candidates, sampler, ho_, objective_)
         ho = Hyperoptimizer(iterations = iters, params = $(esc(params[2:end])), candidates = $(Expr(:tuple, esc.(candidates[2:end])...)), sampler=$(esc(sampler)), objective = objective)
         if $(esc(ho_)) isa Hyperoptimizer # get info from existing ho. the objective function might be changed, due to variables are captured into the closure, so the type of ho also changed.
             ho.sampler = $(esc(ho_)).sampler
-            ho.history = $(esc(ho_)).history
+            ho.history = $(esc(ho_)).history # it's important to use the same array, not copy.
             ho.results = $(esc(ho_)).results
         else
             s = (x->x isa Hyperband ? x.inner : x)(ho.sampler)
@@ -141,17 +141,21 @@ end
 function pmacrobody(ex, params, ho_)
     quote
         function workaround_function()
-            ho = $ho_
-            # hist = copy(ho.history)
+            ho = $(ho_)
+            # Getting the history right is tricky when using workers. The approach I've found to work is to
+            # save the actual array (not copy) in hist, temporarily use a new array that will later be discarded
+            # reassign the original array and then append the new history. If a new array is used, the change will not be visible in the original hyperoptimizer
+            hist = ho.history
+            ho.history = []
             res = pmap(1:ho.iterations) do i
                 $(Expr(:tuple, esc.(params)...)),_ = iterate(ho,i)
                 res = $(esc(ex.args[2])) # ex.args[2] = Body of the For loop
 
                 res, $(Expr(:tuple, esc.(params[2:end])...))
             end
+            ho.history = hist
             append!(ho.results, getindex.(res,1))
-            # ho.history = hist
-            append!(ho.history, getindex.(res,2))
+            append!(ho.history, getindex.(res,2)) # history automatically appended by the iteration
             ho
         end
         workaround_function()
diff --git a/test/runtests.jl b/test/runtests.jl
index f077c2e..10a31bb 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -262,6 +262,7 @@ f(a,b=true;c=10) = sum(@. 100 + (a-3)^2 + (b ? 10 : 20) + (c-100)^2) # This func
     @testset "Parallel" begin
         @info "Testing Parallel"
 
+        rmprocs(workers())
         horp = @phyperopt for i=300, sampler=RandomSampler(), a = LinRange(1,5,50), b = [true, false], c = exp10.(LinRange(-1,3,50))
             # println(i, "\t", a, "\t", b, "\t", c)
             f(a,b,c=c)
@@ -269,6 +270,9 @@ f(a,b=true;c=10) = sum(@. 100 + (a-3)^2 + (b ? 10 : 20) + (c-100)^2) # This func
         @test minimum(horp) < 300
         @test length(horp.history) == 300
         @test length(horp.results) == 300
+        @test all(1:300) do i
+            f(horp.history[i][1:2]..., c=horp.history[i][3]) == horp.results[i]
+        end
 
         Distributed.nworkers() ≤ 1 && addprocs(2)
         @everywhere using Hyperopt