[WIP] Bandits regressors for model selection

[etiennekintzler]

Description

The PR introduce bandits (epsilon-greedy and UCB) for model selection (see issue #270 ). The PR concerns only regressors, but I can add the classifiers in a subsequent PR.

The use of the classes are straightforward :

bandit = UCBRegressor(models=models, metric=metrics.MSE()),

for (x, y) in data.take(N):    
        y_pred = bandit.predict_one(x=x)
        bandit.learn_one(x=x, y=y)

best_model = bandit.best_model

There are convenience methods such as :

• percentage_pulled : to get the percentage each arm was pulled
• best_model : return the model with the highest average reward

Also I added a method add_models where the user can add models on the fly.

I am also working on a notebook that studies the behavior of the bandits for model selection. The notebook also include Exp3, which seems promising but has numerical stability issue and yields counter-intuitive results (see section 3 of the NB). That's why I kept it out of this PR. More generally, the performances of UCB and epsilon-greedy are rather good but there seems to be some variance in the performance.

Improvements

It's still WIP on the following points :

• docstring, mainly add examples + cleaning.
• typing
• some comments might be removed.
• certain functionalities I am not sure to include or not:
  • Save the history of metric value (cf save_metrics parameter) in Bandits
• the name of the classes and the methods are open for changes

I would appreciate inputs regarding the functionalities and the methods/classes naming.

[etiennekintzler]

This part seems to make the tests fail (same part for UCBBandit). Should I enumerate all the parameters (that are in Bandit.__init__) instead of using **kwargs ?

[MaxHalford]

Yes, you should :)

[etiennekintzler]

Actually I try with it but the tests do fail again. I see also that expert.SuccessiveHalving* are purposely exclude from the test (in get_all_estimators) while expert.EWARegressor is not. Is the kind of regressor I implemented supposed to pass this test at all ?

[MaxHalford]

You're right, it's not supposed to pass the tests by default. You need implement a _default_params method, like what is done here. This method will get called here to initialize the state of your model.

[etiennekintzler]

I just add default LinearRegression in a Pipeline and tests pass. It somehow surprise me since I didn't gave default arg for metric and reward_scaler.

Not related to this PR but one test failed :

======================================================================== FAILURES ========================================================================
___________________________________________________ [doctest] river.neighbors.sam_knn.SAMKNNClassifier ___________________________________________________
052     >>> from river import metrics
053     >>> from river import neighbors
054 
055     >>> dataset = synth.ConceptDriftStream(position=500, width=20, seed=1).take(1000)
056 
057     >>> model = neighbors.SAMKNNClassifier(window_size=100)
058 
059     >>> metric = metrics.Accuracy()
060 
061     >>> evaluate.progressive_val_score(dataset, model, metric)
Expected:
    Accuracy: 59.90%
Got:
    Accuracy: 58.20%

/path/to/package/river/neighbors/sam_knn.py:61: DocTestFailure

[etiennekintzler]

@smastelini any idea why the above test is failing?

I could not reproduce that failure in my local setup (in the master branch). Pinging @jacobmontiel, since he has more experience with this specific k-NN variant than I do.

FYU the master branch in my fork is 10 commits behind master in online-ml/river however these 10 commits are mostly doc so..

[MaxHalford]

It passes on my local setup using pytest but it did fail on the CI pipeline (https://travis-ci.org/github/online-ml/river/jobs/745694837).

Very weird! Not sure what to say. Are you sure your development setup is correct?

[etiennekintzler]

@MaxHalford

My bad :/ I forget that I add the bandits classes in the ignored tuple in get_all_estimators.

I can now confirm that it fails as in the CI when I do not provide the arguments.

[jacobmontiel]

@smastelini any idea why the above test is failing?

I could not reproduce that failure in my local setup (in the master branch). Pinging @jacobmontiel, since he has more experience with this specific k-NN variant than I do.

I am confused, is this failing?

[etiennekintzler]

I clone and rebuild the whole package (following https://github.com/online-ml/river/blob/master/CONTRIBUTING.md#installation) and it fails at the same place:

======================================================================== FAILURES ========================================================================
___________________________________________________ [doctest] river.neighbors.sam_knn.SAMKNNClassifier ___________________________________________________
052     >>> from river import metrics
053     >>> from river import neighbors
054 
055     >>> dataset = synth.ConceptDriftStream(position=500, width=20, seed=1).take(1000)
056 
057     >>> model = neighbors.SAMKNNClassifier(window_size=100)
058 
059     >>> metric = metrics.Accuracy()
060 
061     >>> evaluate.progressive_val_score(dataset, model, metric)
Expected:
    Accuracy: 59.90%
Got:
    Accuracy: 58.20%

/home/etienne/PROJETS/river_test/river/river/neighbors/sam_knn.py:61: DocTestFailure

I am using Python 3.7.7 and Ubuntu 20.04.1 LTS.

I can open an issue to keep it out of the PR

[MaxHalford]

Thanks for the PR! It's a good start, and I agree that starting with regression only is a good idea :). I've made a few comments to get the ball rolling. High five!

[MaxHalford]

Yes, you should :)

[etiennekintzler]

Thanks for the PR! It's a good start, and I agree that starting with regression only is a good idea :). I've made a few comments to get the ball rolling. High five!

Thanks, also thank you for this review !

I incorporate all your points and left some conversation unresolved (when your feedback is needed).

I will quickly test this new class since I introduce noticeable change by removing the numpy dependency. Also I need to finish writing docstring (Parameters and Example) then you can merge if it looks good to you. If you see other desirable change tell me too.

[etiennekintzler]

The test check_shuffle_features_no_impact from module estimator_checks fails. The reason is not because of the shuffling of the features per se but because the bandit learning process is not deterministic.

To put it differently, if there is two arms (A1, A2) and two bandits (B1, B2), and if the bandit B1 pull arm A1 and bandit B2 pull arm A2 at round 0, they won't output the same prediction at round 1 because their internals (in particular _average_reward) will differ.

[MaxHalford]

@etiennekintzler, can you close and reopen the pull request? We've switched to GitHub actions for CI, which should be much faster and more enjoyable than Travis :)

[MaxHalford]

@etiennekintzler: you need to a seed parameter and seed the random generation process to make it reproducible.

Regarding unit tests, I guess that for the moment you can rely CI and locally ignore that SamKNN test that isn't passing.