- 
                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 :)  | 
    
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