[Feature Request] Some refactoring of the losses
@vmoens is already working on this.
Since Apr 13, 2023.
- Dominant language
- Python
- Stars
- 3.6k
- Forks
- 487
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 207
Description
Motivation
We want the users to flexibly choose if they want the gradients of the value loss to propagate to common layes in architectures where the actor and value function have tied parameters.
We also want for them to possibly avoid calling twice the common layers.
Some losses ought to be refactored:
-
TD3 should have the option of tying or not the parameters of the actor and qvalue network as in https://github.com/pytorch/rl/pull/1037. We should test whether the loss is backprop on qvalue net or not in the tests
-
Ditto for DDPGLoss
-
Ditto for IQLLoss
-
Ditto for reinforce
-
Ditto for A2C
-
Ditto for SAC
-
Ditto for REDQ (+ deprecated version)
-
TD3 should have the option of having just a value head as in https://github.com/pytorch/rl/pull/1057
Note that this involves NOT cloning the input tensordict for each loss, as they all need access to the previously computed values (see the select and clone removal in #1057) -
Ditto for DDPGLoss
-
Ditto for IQLLoss
-
Ditto for reinforce
-
Ditto for A2C
-
Ditto for SAC
-
Ditto for REDQ (+ deprecated version)
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.