Skip to content

Implement torch.where(condition, x, y) CPU Variable. #4259

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.

Sign up for GitHub

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

Jump to bottom
Merged
gchanan merged 2 commits into from
Dec 20, 2017
Merged

Implement torch.where(condition, x, y) CPU Variable. #4259

gchanan merged 2 commits into from
Dec 20, 2017

Conversation

@gchanan
Copy link
Contributor

@gchanan gchanan commented Dec 19, 2017

This does the following:

  1. Adds a four-argument CPU apply template
  2. Renames scalar template arguments from "CScalar" to "scalar" as previously agreed.
  3. Uses auto in a bunch of places for the existing apply functions
  4. Added a 'BoolTensor' specification in native_functions to indicate it should be a BoolTensor in Declarations.yaml (this matters for Variable type checking)
  5. Implements 'where' using these above; it's more complex than I would like because you want to figure out the broadcasting backwards implicitly and write the derivative only in terms of same size tensors; this requires writing two different functions (where and _s_where) and writing the derivative in terms of _s_where. I can't currently think of an easier way to do this without adding a bunch of mechanism complexity to the native_functions path.

@pytorchbot
Copy link
Collaborator

pytorchbot commented Dec 19, 2017

@gchanan, thanks for your PR! We identified @zdevito to be a potential reviewer.

@gchanan gchanan requested a review from colesbury December 19, 2017 21:42
@onnxbot-worker-2 onnxbot-worker-2 mentioned this pull request Dec 19, 2017
Closed
colesbury
colesbury approved these changes Dec 20, 2017
View reviewed changes
Copy link
Member

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

lgtm

apaszke
apaszke reviewed Dec 20, 2017
View reviewed changes
aten/src/ATen/native/native_functions.yaml
# this allows us to implicitly calculate the broadcast derivative, while only dealing with the
# _s_where derivative.
- func: where(BoolTensor condition, Tensor self, Tensor other) -> Tensor
- func: _s_where(BoolTensor condition, Tensor self, Tensor other) -> Tensor

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

tools/autograd/derivatives.yaml
- name: view(Tensor self, IntList size)
self: grad.contiguous().view(self.sizes())

- name: _s_where(Tensor condition, Tensor self, Tensor other)

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@gchanan gchanan mentioned this pull request Dec 20, 2017
Closed
@gchanan gchanan merged commit 63ac363 into pytorch:master Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang left review comments

@colesbury colesbury colesbury approved these changes

+1 more reviewer

@apaszke apaszke apaszke left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gchanan @pytorchbot @ezyang @colesbury @apaszke