-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Change comparison ops result dtype to bool [part 1] #20767
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@izdeby has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Consider using |
scalar_t, tensor, *tensor_data, sum, UNCERTAIN_TH_OMP_OVERHEAD_THRESHOLD); | ||
return sum; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the code in this block changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and everywhere else in this PR, i just moved methods up above the #if !defined(TH_REAL_IS_BOOL)
to enable them for bool.
The unified diff in this PR is being very unhelpful, because large chunks of code were moved around and the differ doesn't know how to play ball. I'm not sure how to solve this problem; at minimum, it would be helpful to call out where it was just code moved around, and where there were substantive changes (maybe all of this is just code movement?) |
I don't see any tests in this PR? |
@ezyang, good point on calling out what changes exactly were made. i specified which methods were touched but wasn't too clear about what exactly changed. Answering your question, yes it is only code move in this PR (except TH version of sign method, where i add check for bool scalar). Thats why i didnt add any tests as the logic didnt change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume in a later PR you will start actually exercising the bool paths.
Summary: This is the first part of the planned changes to change the comparison operations result tensor dtype from Byte to Bool. You can see the whole list of changes (not cleaned up) [here](pytorch/pytorch#19332). As the PR is too big for a single review im breaking it into pieces. **Changes in this PR:** 1. Enable these methods for bool tensors: - maskedSelect - maskedSelectBool - bitand - cbitand - bitor - cbitor - bitxor - cbitxor - sign - equal - neg 2. Add bool clause for the TH version of sign method. Pull Request resolved: pytorch/pytorch#20767 Differential Revision: D15436446 Pulled By: izdeby fbshipit-source-id: 8d2494b5f4873cd79c7f1a40d2cb045cadfad51a
This is the first part of the planned changes to change the comparison operations result tensor dtype from Byte to Bool. You can see the whole list of changes (not cleaned up) here. As the PR is too big for a single review im breaking it into pieces.
Changes in this PR:
Everything except the TH sign method is just a code movement with no logic change.