Bandit logic refactoring

[MaxHalford]

Hey @etiennekintzler, how are you?

In #736 I did a significant tidy up of the ensemble module. I also renamed the expert module to selection and moved of the methods there into the ensemble module. All in all I'm very happy with the changes.

I took the liberty to refactor the bandit stuff you kindly implemented for us. In fact, I (temporarily) removed UCBRegressor and only included a diluted version of EpsilonGreedyRegressor.

I refactored this code because I wanted to separate the bandit logic from the model selection logic. I hope you'll agree that the seperation of concerns is worthwhile.

Could I ask you to take a look at the bandit.py file if you have some time? Feel free to share any thoughts, or not. I'll move onto to integrating the other good stuff you implemented once I have your approval that the refactor is in the right direction.

Kind regards!

[etiennekintzler]

Hello @MaxHalford I am great thank you ! Hope you're good as well :)

The refactoring makes the whole structure more clean and offers better separation of concerns. Also, the formatting (__repr__) of the bandit looks really good.

However, the class EpsilonGreedy and its mother class BanditSolver doesn't add much in my opinion. The abstract method pull could be defined in Bandit class and implemented in EpsilonGreedyRegressor. You would have all the logic in EpsilonGreedyRegressor and wouldn't have to go back and forth between this class and EpsilonGreedy.

Also the current PR does not implement scaled reward :

river/river/selection/bandit.py

Line 48 in aae8d56

arm.metric.update(y_true, y_pred)

The consequence is that the end result of the selection process is highly dependent on initial conditions (like which arms were pulled the first times). Indeed, take the current example in EpsilonGreedyRegressor that yields:

Index   MAE              Pulls   Share   
    0        32.114043      23    2.30%  
    1        28.959042      23    2.30%  
    2         1.576562     929   92.81%  
    3   623,644.303637      26    2.60%  

If you switch the order of the model 3 and 4 (e.g; learning rates list is now [1e-4, 1e-3, 1e-1, 1e-2]), the best model is not the same :

Index   MAE                    Pulls   Share   
    0               2.654030     921   92.01%  
    1              28.959042      23    2.30%  
    2   5,111,960,455.783644      42    4.20%  
    3              19.523921      15    1.50%  

[MaxHalford]

However, the class EpsilonGreedy and its mother class BanditSolver doesn't add much in my opinion. The abstract method pull could be defined in Bandit class and implemented in EpsilonGreedyRegressor. You would have all the logic in EpsilonGreedyRegressor and wouldn't have to go back and forth between this class and EpsilonGreedy.

That's a good point! Reason why I did: for me a bandit is different that the solver that solves it. A bandit is the problem, a bandit is the algorithm which learns the best arm. But I hear you so I'll take another look and try to find a better design suggest.

Also the current PR does not implement scaled reward

Yes I omitted that part consciously! Thanks for taking the time to provide an example. I'll re-implement this week I think. I'd like to use the TargetScaler class we have instead of doing it ad-hoc.

Cheers!

[etiennekintzler]

for me a bandit is different that the solver that solves it

I would agree it is the case for objects such as optimizers/scalers; for a given model you can pick any optimizer/scaler. However as far as I know the bandit is completely caracterized by the way you solve it (i.e how you pull the arm). To put it differently it wouldn't make sense to use EpsilonGreedyRegressor with a solver that is not EpsilonGreedy(Solver).

Yes I omitted that part consciously! Thanks for taking the time to provide an example. I'll re-implement this week I think.

Ok do no hesitate to tag me !

I'd like to use the TargetScaler class we have instead of doing it ad-hoc.

FYI I didn't find it (I searched on master)

[MaxHalford]

I would agree it is the case for objects such as optimizers/scalers; for a given model you can pick any optimizer/scaler. However as far as I know the bandit is completely caracterized by the way you solve it (i.e how you pull the arm). To put it differently it wouldn't make sense to use EpsilonGreedyRegressor with a solver that is not EpsilonGreedy(Solver).

Right! And yet you may want to use EpsilonGreedy for a different purpose than model selection, no?

Ok do no hesitate to tag me !

Will do!

FYI I didn't find it (I searched on master)

Here you go (gonna move it to preprocessing at some point)

[etiennekintzler]

Right! And yet you may want to use EpsilonGreedy for a different purpose than model selection, no?

Sure, if you plan to use EpsilonGreedy for other purposes, then it makes sense to keep it !

Here you go (gonna move it to preprocessing at some point)

Ok !!