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

Removing _CustomOpTensorDict classes #4

Closed
vmoens opened this issue Feb 10, 2022 · 1 comment
Closed

Removing _CustomOpTensorDict classes #4

vmoens opened this issue Feb 10, 2022 · 1 comment
Assignees
Labels
enhancement New feature or request

Comments

@vmoens
Copy link
Contributor

vmoens commented Feb 10, 2022

_CustomOpTensorDict is an abstract class for some operations (mainly on TensorDict shapes).
It may not be necessary to keep it and maintaining it may be bothersome in the future.

Pros:

  • Most operations that we'd be doing would not change the storage anyway in most cases. For instance, a call on the view method of a regular TensorDict's values will return a view on those tensors, with no extra memory cost.
  • Code clarity: this is one more class that inherits from _TensorDict, and each new operation will (in the current implementation) require its own class. This is not ideal.

Cons:

  • In the case of LazyStackedTensorDict, we would probably like to have a view method that keeps the storage location unchanged. Not using _CustomOpTensorDict will require us to call contiguous on the stack and then call the view operation, but this will assign a new storage to the resulting tensors. The final option could be to prevent users from calling these fixed-storage operations on LazyStackedTensorDict but that could lead to some issues in the future (i.e. users asking for this feature when it has been decided to deprecate it previously).
  • The ProbabilisticOperator class can write to tensordict instances that are unsqueezed. This makes it easy to do in-place modifications of a tensordict without having to create multiple copies of the tensors. This feature is used to execute a policy on a single environment that has no batch size.
@vmoens vmoens added the enhancement New feature or request label Feb 10, 2022
@vmoens vmoens self-assigned this Feb 10, 2022
@vmoens
Copy link
Contributor Author

vmoens commented Apr 12, 2022

Closing as _CustomOpTensorDict is required by a bunch of current components in the code.
Happy to re-open if needed

@vmoens vmoens closed this as completed Apr 12, 2022
vmoens added a commit that referenced this issue Oct 17, 2022
The common practice in the pytorch ecosystem is to type-hint all functions and methods as much as can be.
This PR completes the missing hints in the code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant