-
Notifications
You must be signed in to change notification settings - Fork 550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AWAC implementation #115
AWAC implementation #115
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Just some minor comments (e.g. it'd also be nice to delete some of the commented-out code)
from rlkit.torch.pytorch_util import activation_from_string | ||
|
||
|
||
class CNN(PyTorchModule): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, are you still using this? it could be nice to only use BasicCNN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in a few imports but I am not really using this in recent code. The only file that does seem to use that might be a bit annoying to address is ConvVAE (because a lot of other code uses it). So I think deleting this is best deferred to later?
Also, there's a bunch of files here that we could probably delete since we never use |
Here are the results of this branch on AWAC, SkewFit, and SAC: https://drive.google.com/file/d/1Qy5SYIGNwdeTHAGNjbRfuP5pSiRw8JzJ/view?usp=sharing I went through and did some more cleanup as well. I think this is ready to merge now. |
Everything is running now, but still testing AWAC and skewfit as a regression test.