RLPD algorithm #1727
RLPD algorithm #1727
Conversation
alf/algorithms/rlpd_algorithm.py
Outdated
| critics = critics.reshape(-1, self._num_critic_replicas, | ||
| *self._reward_spec.shape, | ||
| *remaining_shape) | ||
| if self._act_type == ActionType.Discrete: |
There was a problem hiding this comment.
According to __init__(), only ActionType.Continuous is supported
alf/algorithms/rlpd_algorithm.py
Outdated
| if self.has_multidim_reward(): | ||
| sign = self.reward_weights.sign() | ||
| critics = (critics * sign).mean(dim=1) * sign | ||
| else: |
There was a problem hiding this comment.
This branch is unnecessary for 'mean'
There was a problem hiding this comment.
Also curious the motivation of the mean branch? Any benefits?
There was a problem hiding this comment.
This branch is unnecessary for 'mean'
I feel that it is needed in order to get the mean critic values across all critics for actor training?
There was a problem hiding this comment.
Also curious the motivation of the
meanbranch? Any benefits?
This is the default setting of RLPD to get a concensus q_value for actor training. Given that RLPD might use more than two critics, taking a min as in SAC would be too conservative. Moreover, from my previous experience, as long as critics are trained with conservative target critic values, there is no need to be conservative in actor training.
There was a problem hiding this comment.
This branch is unnecessary for 'mean'
I feel that it is needed in order to get the mean critic values across all critics for actor training?
(critics * sign).mean(dim=1) * sign is same as critics.mean(dim=1)
There was a problem hiding this comment.
Oh I see, thanks, updated.
There was a problem hiding this comment.
I had to revert to keep this branch since self.reward_weights by default is None for scalar reward.
There was a problem hiding this comment.
Why can't you change the whole branch as the following?
elif replica_consensus == 'mean':
critics = critics.mean(dim=1)There was a problem hiding this comment.
Oh yes, that should work well.
alf/algorithms/rlpd_algorithm.py
Outdated
| name="RlpdAlgorithm"): | ||
| # **kwargs): | ||
| """ | ||
| Refer to SacAlgorithm for more details for kwargs |
alf/algorithms/rlpd_algorithm.py
Outdated
| Refer to SacAlgorithm for more details for kwargs | ||
|
|
||
| Args: | ||
| name (str): The name of this algorithm. |
| action, | ||
| critics_state, | ||
| replica_consensus='mean', | ||
| sample_subset=False, |
There was a problem hiding this comment.
can add some comments to sample_subset argument, and the reason using True for target critics and False for critics. Seems that using False for critics is to ensure all the critics have gradients.
alf/algorithms/rlpd_algorithm.py
Outdated
|
|
||
| Args: | ||
| name (str): The name of this algorithm. | ||
| num_critic_targets (int): Number of sampled subset of target critics |
There was a problem hiding this comment.
num_critic_targets: maybe can change its name to reflect the sampled aspect? Currently, it is very close to the meaning of num_critic_replicas
There was a problem hiding this comment.
Should ensure that num_critic_targets <= num_critic_replicas.
| critics = critics.permute(*order) | ||
|
|
||
| if sample_subset: | ||
| critics = critics[:, |
There was a problem hiding this comment.
when self._num_critic_targets and num_critic_replicas are equal, the `randperm`` is not necessary and can be removed to save computation.
There was a problem hiding this comment.
Good point, updated.
alf/algorithms/rlpd_algorithm.py
Outdated
| checkpoint=None, | ||
| debug_summaries=False, | ||
| name="RlpdAlgorithm"): | ||
| # **kwargs): |
There was a problem hiding this comment.
can remove this line?
* update actor and critics separately, each with own utd * add an option to use bootstrapped critics
|
Pushed an updated version that works consistently better than previous version. Please take a look. |
Extend SAC to RLPD. Tested on 5 DM control tasks with 4 seeds each to confirmed its performance improvement over SAC.