-
Notifications
You must be signed in to change notification settings - Fork 327
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
[Docs] Fix multi-agent tutorial #1599
Conversation
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.
LGTM, see my comment and feel free to merge if you feel it's the right solution
I don't really have a preference
for tensordict_data in collector: | ||
tensordict_data.set( | ||
("next", "done"), | ||
("next", "agents", "done"), | ||
tensordict_data.get(("next", "done")) | ||
.unsqueeze(-1) | ||
.expand(tensordict_data.get(("next", env.reward_key)).shape), | ||
) # We need to expand the done to match the reward shape (this is expected by the value estimator) | ||
.expand(tensordict_data.get_item_shape(("next", env.reward_key))), | ||
) | ||
tensordict_data.set( | ||
("next", "agents", "terminated"), | ||
tensordict_data.get(("next", "terminated")) | ||
.unsqueeze(-1) | ||
.expand(tensordict_data.get_item_shape(("next", env.reward_key))), | ||
) | ||
# We need to expand the done and terminated to match the reward shape (this is expected by the value estimator) |
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.
what is the best way to approach this?
A common criticism of our tutorials is that they are too complex, I wonder what will be judged as the most complex thing here: (1) boilerplate code such as this, which IMO has little instructive value or (2) an ad-hoc transform that may be a bit too custom to be part of the core lib
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.
Yeah i agree, what i can do is:
- leave as is
- make the transform in the tutorial
- make the transform in core
- We could also consider allowing rewards and dones with different shapes and expanding in the value functionals
What is your preference ?
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.
- seems like a stretch, these classes are already very complex and hard to maintain
I think hese operations are actually 2fold: (1) rename without deleting and (2) unsqueeze + expand
We have a transform that does rename, one that does unsqueeze. We just need one that does expand (or expand_as).
The quickest way forward is this:
Merge as is, open an issue with a feature request for the expand_as transform. Wdyt?
Signed-off-by: Matteo Bettini <matbet@meta.com>
Fix the multi-agent tutorial after the recent "termianted" introduction