Tkurth/rl tests #15
Merged
Tkurth/rl tests #15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… internally from the RL system and the user does not have to bother with it
romerojosh
reviewed
Aug 23, 2024
Collaborator
romerojosh
left a comment
romerojosh
left a comment
There was a problem hiding this comment.
Added mostly nitpick comments here. Otherwise changes LGTM. Thanks for adding some tests!
…, enabled building examples by default
Collaborator
Author
|
I have addressed all your comments and agree to all of them, thanks for the careful review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug in SAC, where instead of alpha was read from the file and used as entropy coefficient, rho was used instead which made training unstable since alpha should be around 0.1 and rho is close to 1.0. In addition to this fix, this PR adds trainable entropy coefficient as it can improve stability of training.
Furthermore, this PR adds a test suite for many parts of the RL stack and in addition a full test for each algorithm (DDPG, TD3, SAC, PPO) on simple tests environments. Those tests run for some time but it is helpful to verify that the environment works. The only test which is currently failing is the one for the state dependent action reward for DDPG, and only for DDPG. This could have something to do with the fact that DDPG sometimes overfits rapidly on simple environments, so I would not assume that the DDPG implementation by itself is wrong. In any case, since TD3 is basically always superior to DDPG and it seems to work in all cases, I recommend using that instead.
In order for the tests to work, I included a gtest dependency and also added some simple RL related models as base models to torch fort. Those include a sac-policy and an actor-critic model, where actor and critic share the same feature extractor.