Skip to content

[Refactor] Make TensorSpec a real class and TensorSpecBase a base class #1222

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vmoens
Copy link
Collaborator

@vmoens vmoens commented Jun 1, 2023

cc @matteobettini any opinion?

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 1, 2023
@vmoens vmoens added Refactoring Refactoring of an existing feature Discussion needed Input from the community is welcome and needed labels Jun 2, 2023
@matteobettini
Copy link
Contributor

matteobettini commented Jun 13, 2023

Do you have two lines on what we are doing here and why? I am not sure I get the full picture from the changelist

@matteobettini
Copy link
Contributor

matteobettini commented Jun 13, 2023

Is it just making TensorSpec a synonym of UnboundedContinuousTensorSpec ?

@vmoens
Copy link
Collaborator Author

vmoens commented Jun 13, 2023

Is it just making TensorSpec a synonym of UnboundedContinuousTensorSpec ?

and creating a base class
it's mildly bc-breaking but I often end up having to write UnboundedContinuousTensorSpec to just say "this is a tensor without precise properties"
Would this make sense? Any cons?

# Conflicts:
#	torchrl/modules/tensordict_module/exploration.py
@matteobettini
Copy link
Contributor

I don’t have a strong opinion about this but two things that might be worth mentioning are:

  • this seems quite a big semantic shift for current users
  • the general TensorSpec term will assume quite a specific instantiation (taking a specific choice among the continuous and discrete options for a spec)

@matteobettini
Copy link
Contributor

Maybe another option is to change UnboundedContinuousTensorSpec to ContinuousTensorSpec and keep TensorSpec as the base

@vmoens
Copy link
Collaborator Author

vmoens commented Jun 13, 2023

Yep the advantages are

  • tensorspecbase is clearer and more aligned with general naming convensions
  • it's mildly bc-breaking as you should not inherit from tensorspec anymore. Besides that, we only create a shortcut so for most users nothing will change. Only type annotation could be impacted.

the general TensorSpec term will assume quite a specific instantiation

What do you mean?

@matteobettini
Copy link
Contributor

What do you mean?

Just that it will be continuous implicitly without having it in the name

@vmoens
Copy link
Collaborator Author

vmoens commented Jun 14, 2023

Not necessarily since you can change the dtype to int or book. It will be unbounded (within the range of the dtype) though

@facebook-github-bot
Copy link

Hi @vmoens!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Discussion needed Input from the community is welcome and needed Refactoring Refactoring of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants