Add observer pattern to implement callbacks

[aprams]

Adds a simple observer pattern, allowing callbacks.
Currently implemented callbacks:

• INIT_DONE_EVENT: Initializaiton (bo.init()) finished
• FIT_STEP_DONE_EVENT: Optimization iteration finished
• FIT_DONE_EVENT: Optimization finished

Sample code (taken from exploitation vs exploration (UCB kappa=1.0 cell) sample for readability):

class Observer:
    def update(self, event, instance):
        print("Observable dispatched event {}".format(event))

observer = Observer()

bo = BayesianOptimization(f=lambda x: f[int(x)],
                          pbounds={"x": (0, len(f)-1)},
                          verbose=0)

bo.register(INIT_DONE_EVENT, observer)
bo.register(FIT_DONE_EVENT, observer)
bo.register(FIT_STEP_DONE_EVENT, observer)

bo.maximize(init_points=2, n_iter=3, acq="ucb", kappa=1, **gp_params)

Output:

Observable dispatched event initialized
Observable dispatched event fit_step_done
Observable dispatched event fit_step_done
Observable dispatched event fit_step_done
Observable dispatched event fit_done

The observer instance is notified via update(self, event, instance), giving it all the flexibility it needs, not limiting any user specific use cases, as you save the results so far already in self.res.

I saw the PR #100, this is basically an advanced version as you requested. I am open for changes to this.

[codecov-io]

Codecov Report

Merging #109 into master will increase coverage by 0.09%.
The diff coverage is 85.29%.

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   82.87%   82.97%   +0.09%     
==========================================
  Files           4        5       +1     
  Lines         292      323      +31     
  Branches       35       38       +3     
==========================================
+ Hits          242      268      +26     
- Misses         45       49       +4     
- Partials        5        6       +1

Impacted Files	Coverage Δ
bayes_opt/__init__.py	100% <100%> (ø)	⬆️
bayes_opt/bayesian_optimization.py	68.8% <100%> (+2.71%)	⬆️
bayes_opt/observer.py	75% <75%> (ø)

---

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 317f735...376f205. Read the comment docs.

[tech4242]

Looks promising! Any reason not to merge this? @fmfn

[tech4242]

@Erotemic can you maybe have a quick glance at this? (I was just looking at the contributor list, hope it's ok that I wrote to you specifically!)

[Erotemic]

I'm by no means an authority on this repo, but I can offer a code review.

After a quick glance, it does look fine to me. Functionality it looks like it works, and the code organization isn't bad.

However I might make a few style suggestions. Feel free to consider them or ignore them.

1. Put the observable code in its own observer.py module. Its a relatively small self-contained bit of code that is a bit more specific than miscellaneous helpers.

2. Create an enum-like class to store all the events instead of adding these constants as global variables. Something like:

class Events(object):
    INIT_DONE = 'initialized'
    FIT_STEP_DONE = 'fit_step_done'
    FIT_DONE = 'fit_done'

This makes it so the events are clearly grouped. They can then be used like
Events.INIT_DONE instead of INIT_DONE_EVENT.

I think the above changes would make the code slightly more readable, but the code also looks merge-able in its current state.

[aprams]

Thanks for the review @Erotemic. Both your suggestions are valid, I thought about that, but didn't want to clutter the slim folder structure. Still you are right, this would help code quality. I will update my contribution.

[aprams]

@Erotemic Thanks for your input! I included both your suggestions. As @fmfn also said that he approves the general idea and the technical side is complete, merging should be no problem, right?

[fmfn]

This is awesome, I really like it, just have a couple questions.

1. The class Events was created but the globals remained. If my understanding is correct, the globals could be removed, right?
2. Can you move Events to either __init__.py or to a dedicated events.py file?
3. What is the purpose of the Observer object? Is it supposed to act as a default observer? Or maybe a parent class so people can build on top of? If so, can we make it more clear?

Thanks for the PR!

[Erotemic]

@fmfn

On 1: Yes, I started a review where I requested that those were removed.

On 3: From what I understand the Observer is supposed to be a parent object. You can see that BayesianOptimization now inherits from it instead of object. Its possible that the Observer could be implemented so it would be an attribute of BayesianOptimization, but in that case it would need to store a parent variable so the callbacks would have access to the underlying object. I'm not sure which design I like better.

On 2: I would actually suggest against this. Because those events belong to the BayesianOptimization observable I think it makes sense to define them within that file.

[aprams]

Thanks for the review!

1. You are right, I forgot to erase the old constants. -> removed them
2. I kinda agree with @Erotemic here, but I am very open to suggestions, I can also move it. -> kept it as it is for now
3. It is kind of a parent object, but it also is really redundant. The implementation basically allows any naming/implementation, even different method names (not just update), so the observer parent class makes almost no sense except for giving an example. (Which we have here and in the test cases as well) -> removed it

[fmfn]

Ok, I was talking about Observer, which has now been removed. I still think it is a good idea to include a barebones Observer to guide users who are trying to implement their own. It doesn't need to be a part of this PR, though, but I can see logging and verbose in general being handled by an Observer like object, would you agree?

As for the location of Events, I'm ok with it not being moved, @Erotemic has a good point.

The only thing left before this is merged is to clean the commits a bit. @aprams could you rebase and squash these down 1-2 commits with a good description?

[aprams]

@fmfn I added references in init.py to have Events and Observer publicly accessible.
Furthermore I squashed the commits into a single one, if you want another commit message or similar, please let me know. I re-added the observer class and gave it a little better default functionality. What do you think?

class Observer:
    def update(self, event, instance):
        # Avoid circular import
        from .bayesian_optimization import Events
        if event is Events.INIT_DONE:
            print("Initialization completed")
        elif event is Events.FIT_STEP_DONE:
            print("Optimization step finished, current max: ", instance.res['max'])
        elif event is Events.FIT_DONE:
            print("Optimization finished, maximum value at: ", instance.res['max'])

[fmfn]

Thank you for the excellent work @aprams and @Erotemic for the comments.