IQVIA-ML / TreeParzen.jl

TreeParzen.jl, a pure Julia hyperparameter optimiser with MLJ integration
Other
35 stars 5 forks source link

Add test to reproduce the issue in “obs_memo” func #87

Closed FatemehTahavori closed 1 year ago

FatemehTahavori commented 1 year ago

Thank you for your contribution. You're a :star: already!

Before submitting this PR, please have a look at the below checklist so that we know more about your PR. Please also reference any relevant issues from the issues page if this PR is intended to address one of those.

What does this PR do?

Add a test case to reproduce the issue in “obs_memo” func from a higher level. For more details about this issue see here https://github.com/IQVIA-ML/TreeParzen.jl/issues/86.

#To show the expected behaviour, the same test in python is added here:  
from hyperopt import Trials, hp, tpe, fmin
from hyperopt.fmin import generate_trials_to_calculate
from hyperopt import space_eval

spaceVar = {'a' : hp.choice('a', [8, 9, 10]),
            'b' : hp.choice('b', [1, 2, 3])}
def objective(space):
    print (space)
    return float(space['b'])/float(space['a'])

best = fmin(fn=objective, space=spaceVar, algo=tpe.suggest, max_evals=30)
space_eval(spaceVar, best)

{'a': 10, 'b': 1}

Checklist

FatemehTahavori commented 1 year ago

@yaxxie could you please give an access to @BojieSheng, he is going to work on fixing this issue.

yaxxie commented 1 year ago

@FatemehTahavori Access isn't required, maintainers can merge PRs. But I don't have control to make those changes.

But regarding the PR, please don't merge as I have comments which I will make later

yaxxie commented 1 year ago

@FatemehTahavori I merged a CI change to include 1.7 and 1.8, please merge it into this branch

FatemehTahavori commented 1 year ago

I know it is failing I will have a look later tonight

yaxxie commented 1 year ago

I know it is failing I will have a look later tonight

No worries thanks for the efforts!

FatemehTahavori commented 1 year ago

I fixed the test but I would like to know what was wrong with previous commit

equal_vals = 0
for i in 21:100
    equal_vals += trials[i].vals[:a] == trials[i].vals[:b] ? 1 : 0
end
equal_vals_percentage = equal_vals * 100 / length(21:100)
BojieSheng commented 1 year ago

I fixed the test but I would like to know what was wrong with previous commit

equal_vals = 0
for i in 21:100
    equal_vals += trials[i].vals[:a] == trials[i].vals[:b] ? 1 : 0
end
equal_vals_percentage = equal_vals * 100 / length(21:100)

I tested this code many times (more than 500 times), have never got this error.

yaxxie commented 1 year ago

Whats the latest on this PR? It is a prerequisite to #90

FatemehTahavori commented 1 year ago

I will close it as this PR https://github.com/IQVIA-ML/TreeParzen.jl/pull/90 has more comprehensive tests