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

Feat: unified gae #1129

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

Feat: unified gae #1129

wants to merge 9 commits into from
+109 −118

Conversation

SimonDuToit
Copy link
Contributor

Makes all PPO systems use a single shared function for calculating the GAE. This is made possible by making the feed forward systems use the dones from the previous step, bringing them in line with the recurrent systems.

sash-a
sash-a requested changes Nov 7, 2024
View reviewed changes
Copy link
Contributor

@sash-a sash-a left a comment

Choose a reason for hiding this comment

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

Looks much nicer! Just some minor nitpicks. Of course we should wait for the final benchmarks, but if that's all good them I'm happy.

Please make an issue for doing this to sable/mat unless you're planning on doing this now (which would be great)

mava/utils/gae.py Outdated Show resolved Hide resolved
mava/utils/gae.py Outdated Show resolved Hide resolved
mava/utils/gae.py Outdated Show resolved Hide resolved
mava/utils/gae.py Outdated Show resolved Hide resolved
mava/utils/gae.py Outdated Show resolved Hide resolved
sash-a
sash-a reviewed Nov 8, 2024
View reviewed changes
Copy link
Contributor

@sash-a sash-a left a comment

Choose a reason for hiding this comment

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

My bad, should be N instead of A for number of agents

mava/utils/multistep.py Outdated Show resolved Hide resolved
mava/utils/multistep.py Outdated Show resolved Hide resolved
SimonDuToit and others added 2 commits November 8, 2024 11:37
@SimonDuToit @sash-a
Replace A with N (number of agents)
f175753 
Co-authored-by: Sasha Abramowitz <reallysasha@gmail.com>
@SimonDuToit @sash-a
same
f6a513e 
Co-authored-by: Sasha Abramowitz <reallysasha@gmail.com>
@OmaymaMahjoub OmaymaMahjoub added benchmark required Docker images get pushed if PR has this label. benchmark complete and removed benchmark required Docker images get pushed if PR has this label. labels Nov 13, 2024
OmaymaMahjoub
OmaymaMahjoub requested changes Nov 13, 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.

@SimonDuToit if you can update MAT and Sable to use gae from this util file 🙏

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

@sash-a sash-a sash-a requested changes

@OmaymaMahjoub OmaymaMahjoub OmaymaMahjoub requested changes

@arnupretorius arnupretorius Awaiting requested review from arnupretorius

@DriesSmit DriesSmit Awaiting requested review from DriesSmit

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

@jcformanek jcformanek Awaiting requested review from jcformanek

@siddarthsingh1 siddarthsingh1 Awaiting requested review from siddarthsingh1

@ulricharmel ulricharmel Awaiting requested review from ulricharmel

@WiemKhlifi WiemKhlifi Awaiting requested review from WiemKhlifi WiemKhlifi 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

@SimonDuToit SimonDuToit

Labels
benchmark complete size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SimonDuToit @sash-a @OmaymaMahjoub