DLR-RM / stable-baselines3

PyTorch version of Stable Baselines, reliable implementations of reinforcement learning algorithms.
https://stable-baselines3.readthedocs.io
MIT License
8.68k stars 1.65k forks source link

Implement Truncated Quantile Critics (TQC) #83

Closed araffin closed 3 years ago

araffin commented 4 years ago

I'm normally against implementing very recent papers before they prove to be valuable but I would like to make an exception for that one, especially because of the good results. It was recently accepted at ICML 2020.

Paper: Controlling Overestimation Bias with Truncated Mixture of Continuous Distributional Quantile Critics Code: https://github.com/bayesgroup/tqc_pytorch

Background

This paper build on SAC, TD3 and QR-DQN, making use of quantile regression to predict a distribution for the value function (instead of a mean value). It truncates the quantiles predicted by different networks (a bit as it is done in TD3). This is for continuous actions only.

Pros

I already implemented it in SB3 (https://github.com/DLR-RM/stable-baselines3/tree/feat/tqc), it was pretty straightforward as I'm using SAC code for the backbone (I did not remove the duplicated code yet) and the authors code for the loss. The difference between SAC and TQC is 30 lines (15 for the loss and 15 for the critic code). And using SAC hyperparameters from the zoo, I could achieve very good results on Pybullet env and on BipedalWalkerHardcore (for this env it reaches maximal performance 10x faster than my previous experiments). The good news is that SAC hyperparameters are transferable to this new algorithm.

The loss function can be re-used to implemented QR-DQN which is apparently a huge improvement over DQN (with minimal effort). The author code is only both in Tensorflow and Pytorch and the results are really good.

Cons

it adds a bit of complexity / duplication but this can be mitigated if it derives from SAC class.

My question is: should we integrate it for v1.0 (#1) or should we wait?

@hill-a @Miffyli @AdamGleave @erniejunior

Miffyli commented 4 years ago

I'm normally against implementing very recent papers before they prove to be valuable but I would like to make an exception for that one, especially because of the good results.

I agree with this one, but the results both on paper and what you say sound promising. This sounds like a solid thing to add to contrib-package once we have that, but I wonder if it could be added to main repo for now (and then later moved to contrib), or should the contrib be created right away? It needs bit of thinking how to do it exactly, but there are other stuff coming up already that could go there (e.g. fancier returns).

Edit: Adding this straight to master is bit iffy, given that even the basic algorithms are still under (slow) review ^^'

AdamGleave commented 4 years ago

My question is: should we integrate it for v1.0 (#1) or should we wait?

@hill-a @Miffyli @AdamGleave @erniejunior

My inclination is to wait for 1.1 since it doesn't seem like an essential feature. If the difference between SAC and QR-DQN is minimal though then I would be fine with having them both in 1.0 if you wanted to put the time in.

vanja-alls commented 4 years ago

учите русский господа я вас не понимаю

30.06.2020, 23:56, Adam Gleave notifications@github.com

My question is: should we integrate it for v1.0 (#1) or should we wait?

@hill-a @Miffyli @AdamGleave @erniejunior

My inclination is to wait for 1.1 since it doesn't seem like an essential feature. If the difference between SAC and QR-DQN is minimal though then I would be fine with having them both in 1.0 if you wanted to put the time in.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

araffin commented 4 years ago

My inclination is to wait for 1.1 since it doesn't seem like an essential feature. If the difference between SAC and QR-DQN is minimal though then I would be fine with having them both in 1.0 if you wanted to put the time in.

Edit: Adding this straight to master is bit iffy, given that even the basic algorithms are still under (slow) review ^^'

I agree with both of you ;) I will wait then and in the meantime, I can still use it by switching to the branch when needed.

araffin commented 3 years ago

Issue moved to contrib repo

araffin commented 3 years ago

Now available in https://github.com/Stable-Baselines-Team/stable-baselines3-contrib