Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[BugFix] Fix TD3 target net #1186

Merged
merged 13 commits into from
May 23, 2023
Merged

[BugFix] Fix TD3 target net #1186

merged 13 commits into from
May 23, 2023

Conversation

vmoens
Copy link
Contributor

@vmoens vmoens commented May 23, 2023 •
edited
Loading

No description provided.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 23, 2023
@vmoens vmoens added the bug Something isn't working label May 23, 2023
@vmoens
Copy link
Contributor Author

vmoens commented May 23, 2023

@BY571 I made some more changes and I have a good training curve now
Can you check if it all makes sense?

@BY571
Copy link
Contributor

BY571 commented May 23, 2023

@BY571 I made some more changes and I have a good training curve now Can you check if it all makes sense?

looking good! Also getting the "old" performance now with the correct delayed values :)

@vmoens
Copy link
Contributor Author

vmoens commented May 23, 2023

@BY571 I made some more changes and I have a good training curve now Can you check if it all makes sense?

looking good! Also getting the "old" performance now with the correct delayed values :)

Do you have the permission make a review? That would be great!

BY571
BY571 reviewed May 23, 2023
View reviewed changes
examples/td3/config.yaml
total_frames: 1000000
frames_per_batch: 1000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big issue but I might put this back to 1000 as its a common length for MuJoCo envs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure
this is the frames in each batch but not the truncation length.
This should be handled by max_frames_per_traj

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes! I was thinking of env like halfcheetah. But you are right for Hopper or others where episodes can be less than 1000 so multiple epochs per batch!

@BY571
Copy link
Contributor

BY571 commented May 23, 2023

looks good to me now!

@vmoens vmoens merged commit 4ece06c into main May 23, 2023
@vmoens vmoens deleted the fix_td3 branch May 23, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BY571 BY571 BY571 left review comments

Assignees
No one assigned
Labels
bug Something isn't working CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vmoens @BY571 @facebook-github-bot