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: general wrapper framework #948

Merged
merged 12 commits into from
Nov 30, 2023
Merged

feat: general wrapper framework #948

merged 12 commits into from
Nov 30, 2023

Conversation

sash-a
Copy link
Contributor

@sash-a sash-a commented Nov 21, 2023

What?

General observation and example rware wrapper which makes it easy to support more jumanji envs in the future 👀

Why?

To support more jumanji envs

How?

Create an observation type with known fields. Wrap all new envs so that they return this observation type instead of their own custom observation.

sash-a
sash-a commented Nov 21, 2023
View reviewed changes
mava/wrappers/jumanji.py Outdated Show resolved Hide resolved
sash-a
sash-a commented Nov 24, 2023
View reviewed changes
Copy link
Contributor Author

@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.

I can't approve 😢 but I am happy with this, just one question about the shared reward and a minor docstring thing

mava/types.py Show resolved Hide resolved
mava/types.py Outdated Show resolved Hide resolved
mava/wrappers/jumanji.py Outdated Show resolved Hide resolved
mava/types.py Show resolved Hide resolved
mava/types.py Outdated Show resolved Hide resolved
@sash-a
Copy link
Contributor Author

sash-a commented Nov 24, 2023

Just for bookkeeping #951 was discovered in this PR, but we're deciding to fix it in a separate PR so we can get this one in ASAP.

@WiemKhlifi WiemKhlifi mentioned this pull request Nov 24, 2023
Merged
OmaymaMahjoub
OmaymaMahjoub requested changes Nov 27, 2023
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.

Can we add a feature called use_team_reward as we need it for a fair comparison between our work and existing works that rely on giving the agents the same reward measured by mean(timestep.reward)

@WiemKhlifi
Copy link
Contributor

I made all the necessary changes in this PR #952, just to make this PR smaller.

Can we add a feature called use_team_reward as we need it for a fair comparison between our work and existing works that rely on giving the agents the same reward measured by mean(timestep.reward)

RuanJohn
RuanJohn requested changes Nov 29, 2023
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 for all the work! Super nice changes! Just some minor comments and suggestions here and there.

mava/evaluator.py Show resolved Hide resolved
mava/systems/rec_ippo_rware.py Outdated Show resolved Hide resolved
mava/systems/rec_ippo_rware.py Outdated Show resolved Hide resolved
mava/systems/rec_mappo_rware.py Outdated Show resolved Hide resolved
mava/types.py Outdated Show resolved Hide resolved
mava/types.py Outdated Show resolved Hide resolved
mava/types.py Outdated Show resolved Hide resolved
mava/types.py Outdated Show resolved Hide resolved
mava/wrappers/jumanji.py Show resolved Hide resolved
mava/wrappers/jumanji.py Outdated Show resolved Hide resolved
RuanJohn
RuanJohn reviewed Nov 29, 2023
View reviewed changes
mava/types.py Outdated Show resolved Hide resolved
@sash-a sash-a mentioned this pull request Nov 29, 2023
Merged
RuanJohn
RuanJohn approved these changes Nov 30, 2023
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 @WiemKhlifi and @sash-a 🔥

OmaymaMahjoub
OmaymaMahjoub approved these changes Nov 30, 2023
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.

Thanks @WiemKhlifi and @sash-a 🔥

@RuanJohn RuanJohn merged commit 23f45ea into develop Nov 30, 2023
4 checks passed
@RuanJohn RuanJohn deleted the feat/general-wrapper-framework branch November 30, 2023 07:51
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RuanJohn RuanJohn RuanJohn approved these changes

@WiemKhlifi WiemKhlifi WiemKhlifi left review comments

@OmaymaMahjoub OmaymaMahjoub OmaymaMahjoub approved these changes

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

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

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

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

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

Assignees

@sash-a sash-a

@WiemKhlifi WiemKhlifi

Labels
size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sash-a @WiemKhlifi @RuanJohn @OmaymaMahjoub