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

simplify Feature implementation #5539

Merged
merged 2 commits into from
Mar 3, 2022
Merged

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Mar 3, 2022

This removes the __init_subclass__() and _to_tensor() methods of _Feature. __init_subclass__() was inspecting the MRO and dynamically setting attribute of class instances through the _META_ATTRS and _metadata attributes which are now removed. Instead, we now add new_like() class method to all _Feature subclasses: slightly more verbose, but much simpler.

@facebook-github-bot
Copy link

facebook-github-bot commented Mar 3, 2022

💊 CI failures summary and remediations

As of commit ef2efb1 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Copy link
Collaborator Author

@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.

Apart from two previously undetected typing errors that were surfaced by this patch, all changes only happen in the prototype.features module. Given that all tests are passing and mypy is also happy, I'm guessing that this change is indeed functional equivalent.

cls._to_tensor(data, dtype=dtype, device=device),
# requires_grad
False,
torch.as_tensor(data, dtype=dtype, device=device), # type: ignore[arg-type]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed offline, the annotations of as_tensor are wrong. str and int are valid types for the device.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @pmeier , LGTM. Would you mind adding a description to the PR, something like (feel free to edit)

This removes the __init_subclass__() and _to_tensor() methods of _Feature. __init_subclass__() was inspecting the mro and dynamically setting attribute of class instances through the _META_ATTRS and _metadata attributes which are now removed. Instead, we now add new_like() class method to all _Feature subclasses: slightly more verbose, but much simpler.

@pmeier pmeier merged commit e836b3d into pytorch:main Mar 3, 2022
@pmeier pmeier deleted the simplify-features branch March 3, 2022 14:41
@github-actions
Copy link

github-actions bot commented Mar 3, 2022

Hey @pmeier!

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 Mar 15, 2022
Summary:
* simplify Feature implementation

* fix mypy

Reviewed By: vmoens

Differential Revision: D34878978

fbshipit-source-id: 765518672e48d53d1f61e5d71c3f7dddc902948c
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