-
Notifications
You must be signed in to change notification settings - Fork 326
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.
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
[Algorithm] QMixer loss and multiagent models #1378
Conversation
@Acciorocketships it would be nice to add a QGNN mixer in modules/models/multiagent.py. |
What about the QGNN model? If that's not already implemented, I would prioritise that. It has a much larger impact on performance. |
Yeah that is just a normal gnn right? I am thinking to make a MultiagentGNN as i made the MLP. I can take care of that but the mixer is up to you |
Ready for review @vmoens |
Thanks a lot! Will fix. Most of the comments on the new loss are on parts that have been copy-pasted from DQN. |
You mean the loss_function default? Yeah sure makes sense |
Not only the default, basically all of the comments. So your review comments apply to dqnloss too. |
Oh i see what you mean |
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
Signed-off-by: Matteo Bettini <matbet@meta.com>
@in_keys.setter | ||
def in_keys(self, values): | ||
self._in_keys = values | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change you requested makes it bc-breaking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want me to still allow it but warn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no if tests pass without needing it we're good without it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thin they pass
Tests are failing on GPU tests:
|
oh i am seeing now that some are failing |
Signed-off-by: Matteo Bettini <matbet@meta.com>
There is still this to be fixed
|
Thanks a lot and sorry for not noticing that. For the future, can you explain to me where do you see such failures? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Woohoo!
No description provided.