Skip to content

prevent feature wrapping if the feature is not the primary operand #6095

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

Merged
merged 4 commits into from
Sep 30, 2022

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented May 26, 2022

Fixes #6094.

Apart from the fix, this PR also adds a few minimal tests that I copied from the design doc, to ensure I'm not breaking anything else.

assert label_to.categories is label.categories


def test_to_feature_reference():
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the test case that makes sure we actually fix #6094.

Comment on lines +84 to +85
if not isinstance(args[0], cls):
return output
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Although the behavior should be kept for all ops where we might wrap, right now this only applies to torch.Tensor.to. torch.Tensor.clone does not accept any inputs and thus this method will only be invoked in case the data to clone is actually a feature. All other ops will be handled by the else branch which performs no wrapping.

Thus, we could also merge this into the torch.Tensor.to branch for now, but will need to put it back where it is right now as soon as we special case another op that takes tensors as inputs. I would keep it were it is, but won't oppose putting it elsewhere for now.

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Let's finally merge this, thanks @pmeier !

@pmeier pmeier merged commit 7f4c55b into pytorch:main Sep 30, 2022
@pmeier pmeier deleted the feature-wrapping branch September 30, 2022 08:32
@github-actions
Copy link

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 Oct 7, 2022
…perand (#6095)

Summary:
* prevent feature wrapping if the feature is not the primary operand

* explicitly add feature tests to CI

Reviewed By: datumbox

Differential Revision: D40138743

fbshipit-source-id: b5e523ce612b7380f8f9d11565f39b0fb6ef7b22
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.

Tensor.to does not work with feature as reference
3 participants