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

Remove wrap_like class method and add datapoints.wrap() function #7832

Merged
merged 7 commits into from
Aug 16, 2023

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Aug 15, 2023

@NicolasHug NicolasHug requested a review from pmeier August 15, 2023 14:30
Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor comments. Otherwise, LGTM. Thanks Nicolas!

@@ -12,3 +12,25 @@
import warnings

warnings.warn(_BETA_TRANSFORMS_WARNING)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not going to block over this, but I would prefer keeping logic out of __init__.py.

@@ -12,3 +12,25 @@
import warnings

warnings.warn(_BETA_TRANSFORMS_WARNING)


def wrap(wrappee, *, like, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def wrap(wrappee, *, like, **kwargs):
D = TypeVar("D", bounds=Datapoint)
def wrap(wrappee: torch.Tensor, *, like: D, **kwargs: Any) -> D:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤮

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had merged the suggestion and fixed the imports but it created more problems in the BBox._wrap, so I'm getting rid of it. Mypy is green anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a look.

@NicolasHug NicolasHug merged commit c1592f9 into pytorch:main Aug 16, 2023
@github-actions
Copy link

Hey @NicolasHug!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

facebook-github-bot pushed a commit that referenced this pull request Aug 25, 2023
…tion (#7832)

Summary: Co-authored-by: Philip Meier <github.pmeier@posteo.de>

Reviewed By: matteobettini

Differential Revision: D48642267

fbshipit-source-id: 43331507612c8980dd275aaaa40d38801a3c95c9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants