Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Remove the unnecessary regular advantage code in PPO #287

Merged
merged 18 commits into from
Oct 4, 2022
Merged

Remove the unnecessary regular advantage code in PPO #287

merged 18 commits into from
Oct 4, 2022

Conversation

bragajj
Copy link
Contributor

@bragajj bragajj commented Oct 3, 2022 •
edited by vwxyzjn
Loading

Description

Resolving issue #207
Unnnecessary ppo code removed, numerical accuracy was ensured by team members through debugger. Additional runs showing performance without the extra code can be found at the following wandb link: https://wandb.ai/bragajj/ppo_advcalc

Types of changes

  • Bug fix
  • New feature
  • New algorithm
  • Documentation

Checklist:

  • I've read the CONTRIBUTION guide (required).
  • I have ensured pre-commit run --all-files passes (required).

bragajj added 7 commits October 3, 2022 14:15
@bragajj
Update ppo.py
c242e9c 
To resolve issue vwxyzjn#207 in cleanrl, extra advantage code not needed
@bragajj
Update ppo_atari.py
1770542 
To resolve issue vwxyzjn#207 in cleanrl, extra advantage calc code unnecessary
@bragajj
Update ppo_atari_envpool.py
595d48c 
Updated to resolve issue vwxyzjn#207, unncessary additional advantage calc code
@bragajj
Update ppo_continuous_action.py
46a1839 
Updated to resolve issue vwxyzjn#207
@bragajj
Update ppo_atari_lstm.py
465dee6 
Updated to resolve issue vwxyzjn#207, unnecessary additional advantage calc code for ppo implementations
@bragajj
Update ppo_pettingzoo_ma_atari.py
fd3665a 
Updated to resolve issue vwxyzjn#207
@bragajj
Update ppo_procgen.py
b0994b7 
Updated to resolve issue vwxyzjn#207
@vercel
Copy link

vercel bot commented Oct 3, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cleanrl ✅ Ready (Inspect) Visit Preview Oct 4, 2022 at 0:52AM (UTC)

@vwxyzjn vwxyzjn changed the title Issue #207 Resolution PPO Remove the unnecessary regular advantage code in PPO Oct 3, 2022
@vwxyzjn
Copy link
Owner

vwxyzjn commented Oct 3, 2022

Thanks @bragajj, it looks good. I would also remove the gae flag.

cleanrl/cleanrl/ppo_atari.py

Lines 58 to 59 in 49168b8

parser.add_argument("--gae", type=lambda x: bool(strtobool(x)), default=True, nargs="?", const=True,
help="Use GAE for advantage computation")

The --gae flag exists in other scripts as well. If you could do the same for them, that would be great!

image

@bragajj
Copy link
Contributor Author

bragajj commented Oct 3, 2022

GAE flags removed from all ppo files, isaac gym and ppo_rnd_envpool.py are also now updated to reflect GAE revisions

@bragajj
Update ppo_rnd_envpool.py
06c5507 
Fixed styling of lines 432-436
vwxyzjn
vwxyzjn approved these changes Oct 4, 2022
View reviewed changes
Copy link
Owner

@vwxyzjn vwxyzjn left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @bragajj!

@vwxyzjn vwxyzjn merged commit f0bbf49 into vwxyzjn:master Oct 4, 2022
@vwxyzjn vwxyzjn mentioned this pull request Oct 4, 2022
Closed
@vwxyzjn vwxyzjn mentioned this pull request Oct 19, 2022
Closed
vwxyzjn added a commit to jseppanen/cleanrl that referenced this pull request Nov 20, 2022
@vwxyzjn
Remove duplicate adv calculation (see vwxyzjn#287)
c052f44
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@vwxyzjn vwxyzjn vwxyzjn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bragajj @vwxyzjn