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.

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

[Feature Request] Make objective modules compatible with dictionaries #12

Closed
vmoens opened this issue Feb 10, 2022 · 2 comments
Closed
Assignees
Labels
enhancement New feature or request quality code quality

Comments

@vmoens
Copy link
Contributor

vmoens commented Feb 10, 2022

The usage of the TensorDict class simplifies the process of passing data across processes, designing general classes that are oblivious to the keys used in a specific algorithm (e.g. whether or not a action_log_prob / hidden_state key should be expected).
However, introducing new classes can prevent users from copy-paste and re-use modules (see here). We should make sure that TensorDict is used only when absolutely necessary. These cases include situations where all the content of a dictionary will be treated in a similar way:

  • indexing
  • reshaping
  • sending from worker to worker, device to device
  • concatenation / stacking
    In general, TensorDict should be used for high-level classes: Agent, DataCollector, possibly probabilistic operator modules.

Objectives should not require TensorDicts in general.

However, in some cases they may need to check the trajectory length (1st dimension) or the batch size (0th dimension), or even the device. An option in those cases would be to infer those from a specific tensor in the dictionary (e.g. reward?)

Plan

  • Test and fix modules such that they all accept a dictionary as input.
  • Modify typing in this perspective.
  • Currently, modules return a TensorDict but we could perfectly return a regular dict.
@vmoens vmoens added enhancement New feature or request quality code quality labels Feb 10, 2022
@vmoens vmoens self-assigned this Feb 10, 2022
@vmoens
Copy link
Contributor Author

vmoens commented Apr 13, 2022

It's unclear if this would be necessary after all.
If actively asked by the community i'd be happy to re-open.

@vmoens vmoens closed this as completed Apr 13, 2022
@vmoens vmoens reopened this May 9, 2022
@Benjamin-eecs Benjamin-eecs changed the title Make objective modules compatible with dictionaries [Feature Request] Make objective modules compatible with dictionaries Jul 21, 2022
@vmoens
Copy link
Contributor Author

vmoens commented Aug 29, 2022

Won't fix for now as we need batch size for multiple losses.

@vmoens vmoens closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request quality code quality
Projects
None yet
Development

No branches or pull requests

1 participant