-
Notifications
You must be signed in to change notification settings - Fork 412
Adds perceptive actor-critic class #114
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
base: main
Are you sure you want to change the base?
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.
Hey @pascal-roth, very cool, thanks a lot!
22cd1ec to
8a0a959
Compare
rsl_rl/networks/cnn.py
Outdated
| if self.flatten: | ||
| x = x.flatten(start_dim=1) | ||
| elif self.avgpool is not None: | ||
| x = self.avgpool(x) | ||
| x = x.flatten(start_dim=1) | ||
| return x |
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.
I don't get this logic. Why do we always flatten when we do avgpool and why do we not do avgpool when we also set the flatten flag?
61038de to
d69435a
Compare
42e406d to
df9e1ad
Compare
d69435a to
c871703
Compare
df9e1ad to
0a1756d
Compare
|
@pascal-roth I modified the CNN quite a bit to have more flexibility. Could you quickly check if this version works for you and maybe also give it a quick check? I also modified your perceptive environment configs to work with the changes here, it would be awesome if you could run the env to see if it still trains the same :) |
|
Hello! I was following the progress of this PR and I had a question whether it makes sense to extend this ActorCriticPerceptive to have a shared CNN backbone for both critic and actor? Do you think it should be an option or should it be needed at all? I thought it can greatly simplify the complexity of neural network and speed up the training process. |
|
Hi @lvjonok, I would assume the speedup is negligible. But if you want to test this please share the results :) |
|
Hi @ClemensSchwarke in my tests, it seems using a shared CNN backbone between actor and critic can help save VRAM. I find this particularly helpful to use more environments per GPU |
Adds a new perceptive actor-critic class, that can define CNN layers for every 2D observation term.
Example usage shown in IsaacLab isaac-sim/IsaacLab#3467