Skip to content

Introducing IsInf #9169

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
Closed
bhushan23 wants to merge 1 commit into from
Closed

Introducing IsInf #9169

bhushan23 wants to merge 1 commit into from

Conversation

@bhushan23
Copy link
Contributor

@bhushan23 bhushan23 commented Jul 4, 2018

torch.isinf - checks element wise +/- inf implements #9132

@bhushan23 bhushan23 mentioned this pull request Jul 4, 2018
Closed
@bhushan23
Copy link
Contributor Author

bhushan23 commented Jul 4, 2018

@ssnl @vishwakftw please review

@vishwakftw
Copy link
Contributor

vishwakftw commented Jul 4, 2018 •
edited
Loading

Please add tests in test_torch.py. You can follow the tests for isnan.

@bhushan23
Copy link
Contributor Author

bhushan23 commented Jul 4, 2018 •
edited
Loading

test_isinf added, I couldn't find the use of test function, can you point to the uses? There are some places where isnan is used within other test cases, should we also add in there?

@ssnl
Copy link
Collaborator

ssnl commented Jul 6, 2018

test_isinf doesn't seem added

@bhushan23
Copy link
Contributor Author

bhushan23 commented Jul 6, 2018

Oh! It was missed! added !!
We only need to create test method in TestTorch?

@vishwakftw
Copy link
Contributor

vishwakftw commented Jul 6, 2018

Add the function to docs/source/torch.rst, just like torch.isnan.

@bhushan23 bhushan23 requested review from ssnl and zou3519 as code owners July 6, 2018 17:05
@bhushan23
Copy link
Contributor Author

bhushan23 commented Jul 6, 2018

Thanks @vishwakftw done

@bhushan23
Copy link
Contributor Author

bhushan23 commented Jul 6, 2018

@ssnl @zou3519 can you approve the change?

ssnl
ssnl reviewed Jul 6, 2018
View reviewed changes
torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@bhushan23
Copy link
Contributor Author

bhushan23 commented Jul 9, 2018 •
edited
Loading

@ssnl done

zou3519
zou3519 reviewed Jul 9, 2018
View reviewed changes
test/test_torch.py Outdated

This comment was marked as off-topic.

Sign in to view

zou3519
zou3519 reviewed Jul 9, 2018
View reviewed changes
torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

zou3519
zou3519 reviewed Jul 9, 2018
View reviewed changes
torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

zou3519
zou3519 reviewed Jul 9, 2018
View reviewed changes
torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

zou3519
zou3519 reviewed Jul 9, 2018
View reviewed changes
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

The code for isinf is a one-liner, so I'm not sure it is worth adding it to the API.

Other than that, I left a few nits below: the code looks good.

@ssnl
Copy link
Collaborator

ssnl commented Jul 9, 2018

We have isnan so isinf seems fine. Also np has it IIRC. We should move them to ATen one day though.

facebook-github-bot
facebook-github-bot reviewed Jul 9, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@vadimkantorov
Copy link
Contributor

vadimkantorov commented Jul 9, 2018

isfinite should also be a one-liner (at least an inefficient one) - it's also supported by numpy/tf, but with isinf the pain is already less anyway :)

@bhushan23
Copy link
Contributor Author

bhushan23 commented Jul 10, 2018

@zou3519

  1. Dumping input in case not a tensor
  2. Updated comment
  3. Keeping ByteTensor sentence in comment for consistency with other methods and also because it is indeed ByteTensor. We are stating in next line that input is either 1/0

@bhushan23
Copy link
Contributor Author

bhushan23 commented Jul 11, 2018

@zou3519 I am planning to open new issue where we can change comment of all functional methods such as isnan to be consistent with new output format. Right now, would like to go ahead with current version

@vishwakftw
Copy link
Contributor

vishwakftw commented Jul 11, 2018

This has already been fixed in a previous PR, I believe.

vishwakftw
vishwakftw reviewed Jul 11, 2018
View reviewed changes
torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

vishwakftw
vishwakftw reviewed Jul 11, 2018
View reviewed changes
docs/source/torch.rst Outdated

This comment was marked as off-topic.

Sign in to view

vishwakftw
vishwakftw reviewed Jul 11, 2018
View reviewed changes
torch/functional.py Outdated

This comment was marked as off-topic.

Sign in to view

@bhushan23
Copy link
Contributor Author

bhushan23 commented Jul 11, 2018

Thanks @vishwakftw for pointing out spacing and order issue

@bhushan23
Introducing torch.isinf
2397bcc 
Summary:
Given tensor, checks element wise +/- inf and returns ByteTensor

Test added: test_isinf
Reviewed by: SsnL, vishwakftw, zou3519
@bhushan23
Copy link
Contributor Author

bhushan23 commented Jul 13, 2018 •
edited
Loading

@zou3519 are we done here?

@zou3519
Copy link
Contributor

zou3519 commented Jul 13, 2018

@bhushan23 thank you for the reminder. Let me give this one more look through.

zou3519
zou3519 approved these changes Jul 13, 2018
View reviewed changes
Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

LGTM!

facebook-github-bot
facebook-github-bot reviewed Jul 13, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@bhushan23
Copy link
Contributor Author

bhushan23 commented Jul 13, 2018

@apaszke @colesbury @ezyang @gchanan @soumith need one of yours approval too

petrex pushed a commit to petrex/pytorch that referenced this pull request Jul 16, 2018
Merge remote-tracking branch 'upstream/master' into hipify_c2
986eeb7 
* upstream/master: (24 commits)
  Implement tensor weak references (pytorch#9363)
  Nuke TestCollectEnv (pytorch#9459)
  Add test case for segmentation fault fix in grad_fn (pytorch#9457)
  Add peephole optimization for type_as operators. (pytorch#9316)
  Fix out-of-range error for test_neg (pytorch#9431)
  add depthwise conv support for mkldnn (pytorch#8782)
  Refactor `_log_sum_exp` (pytorch#9173)
  Add ModuleDict and ParameterDict containers (pytorch#8463)
  Introduce SupervisedPtr, delete THAllocator and THCDeviceAllocator (pytorch#9358)
  Introducing IsInf (pytorch#9169)
  add device to CUDAEvent (pytorch#9415)
  Make localScalar error message more intuitive (pytorch#9443)
  Only accept continguous tensors in TopK for cuda (pytorch#9441)
  Add support for .norm() pytorch onnx export and ReduceL1/ReduceL2 caffe2 operators (pytorch#9299)
  Only view() rhs of index_put if we need to (pytorch#9424)
  Add BatchBucketizeOp in caffe2 (pytorch#9385)
  Implementation of Wngrad optimizer caffe2 python wrapper and unit test on least square regression (pytorch#9001)
  Implementation and operator test for Wngrad optimizer (pytorch#8999)
  Fix segmentation fault in grad_fn (pytorch#9292)
  update docs (pytorch#9423)
  ...
goldsborough pushed a commit to goldsborough/pytorch that referenced this pull request Jul 20, 2018
@bhushan23 @goldsborough
Introducing IsInf (pytorch#9169)
c7d781e 
Summary:
torch.isinf - checks element wise +/- inf implements pytorch#9132
Pull Request resolved: pytorch#9169

Reviewed By: SsnL

Differential Revision: D8768614

Pulled By: zou3519

fbshipit-source-id: dd1b5f6c976deb421d626e22cdd25500ec04d796
@bhushan23 bhushan23 deleted the isInf_Change branch July 25, 2018 19:39
jramseyer pushed a commit to jramseyer/pytorch that referenced this pull request Jul 30, 2018
@bhushan23 @jramseyer
Introducing IsInf (pytorch#9169)
c21388f 
Summary:
torch.isinf - checks element wise +/- inf implements pytorch#9132
Pull Request resolved: pytorch#9169

Reviewed By: SsnL

Differential Revision: D8768614

Pulled By: zou3519

fbshipit-source-id: dd1b5f6c976deb421d626e22cdd25500ec04d796
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
@bhushan23
Introducing IsInf (pytorch#9169)
949000d 
Summary:
torch.isinf - checks element wise +/- inf implements pytorch#9132
Pull Request resolved: pytorch#9169

Reviewed By: SsnL

Differential Revision: D8768614

Pulled By: zou3519

fbshipit-source-id: dd1b5f6c976deb421d626e22cdd25500ec04d796
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ssnl ssnl ssnl left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@ezyang ezyang ezyang approved these changes

@zou3519 zou3519 zou3519 approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@colesbury colesbury Awaiting requested review from colesbury

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

+1 more reviewer

@vishwakftw vishwakftw vishwakftw left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@bhushan23 @vishwakftw @ssnl @vadimkantorov @zou3519 @ezyang @facebook-github-bot