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

chore: PPO system cleanup #1124

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

chore: PPO system cleanup #1124

wants to merge 20 commits into from
+308 −540

Conversation

sash-a
Copy link
Contributor

@sash-a sash-a commented Nov 1, 2024

What?

Did a couple small clean ups to the PPO systems that are long overdue:

  • Removed all the capital comments
  • Removed opt states from the losses
  • Removed tree map around the done reshape during acting (it's just an array)
  • Removed metrics from the traj batch because they aren't really part of the batch
  • Cleaner unpacking of the loss metrics

@RuanJohn should we add this for MAT?

@sash-a sash-a self-assigned this Nov 1, 2024
RuanJohn
RuanJohn requested changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@RuanJohn RuanJohn left a comment

Choose a reason for hiding this comment

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

Thank you @sash-a! I think these are some very needed changes. I just have a few suggestions and questions. Any comment applies to all systems.

mava/systems/mat/anakin/mat.py Show resolved Hide resolved
mava/systems/mat/anakin/mat.py Outdated Show resolved Hide resolved
mava/systems/mat/anakin/mat.py Show resolved Hide resolved
mava/systems/ppo/anakin/ff_ippo.py Outdated Show resolved Hide resolved
mava/systems/ppo/anakin/ff_ippo.py Outdated Show resolved Hide resolved
mava/systems/ppo/anakin/ff_mappo.py Show resolved Hide resolved
mava/systems/ppo/anakin/ff_mappo.py Show resolved Hide resolved
@sash-a
Copy link
Contributor Author

sash-a commented Nov 7, 2024

todo: same fixes for sable and switch sable to using the PPOTransition

WiemKhlifi
WiemKhlifi reviewed Nov 11, 2024
View reviewed changes
README.md Show resolved Hide resolved
@OmaymaMahjoub OmaymaMahjoub added the benchmark required Docker images get pushed if PR has this label. label Nov 13, 2024
OmaymaMahjoub
OmaymaMahjoub approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@OmaymaMahjoub OmaymaMahjoub left a comment

Choose a reason for hiding this comment

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

LGTM! Let's just wait for the benchmark before we merge this 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WiemKhlifi WiemKhlifi WiemKhlifi left review comments

@RuanJohn RuanJohn RuanJohn requested changes

@OmaymaMahjoub OmaymaMahjoub OmaymaMahjoub approved these changes

@arnupretorius arnupretorius Awaiting requested review from arnupretorius

@DriesSmit DriesSmit Awaiting requested review from DriesSmit

@jcformanek jcformanek Awaiting requested review from jcformanek

@siddarthsingh1 siddarthsingh1 Awaiting requested review from siddarthsingh1

@ulricharmel ulricharmel Awaiting requested review from ulricharmel

@SimonDuToit SimonDuToit Awaiting requested review from SimonDuToit SimonDuToit is a code owner

@Louay-Ben-nessir Louay-Ben-nessir Awaiting requested review from Louay-Ben-nessir Louay-Ben-nessir is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@sash-a sash-a

Labels
benchmark required Docker images get pushed if PR has this label. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sash-a @RuanJohn @OmaymaMahjoub @WiemKhlifi @Louay-Ben-nessir