torch.histc added a finite range check to resolve segfaults if tensor has inf. also added checks for nan values, min>max #27712

anjali411 · 2019-10-10T18:36:29Z

nairbv · 2019-10-10T18:41:39Z

aten/src/TH/generic/THTensorMoreMath.cpp

  }

+  TORCH_CHECK(!(std::isinf(minval) || std::isinf(maxval)), "range of [", minval, ", ", maxval, "] is not finite");
+


looks good, let's also add the example from the issue to a unit test too

vadimkantorov

Will it be fine for NaN?

anjali411 · 2019-10-11T16:25:58Z

@vadimkantorov good point! I added a check for that and it will throw an error for tensors with nan values in case a finite range is not specified

test/test_torch.py

yf225

Changes look awesome! CI failure is likely unrelated, after rebasing and passing CI we should be good to go.

yf225 · 2019-10-18T17:49:27Z

aten/src/ATen/native/cuda/SummaryOps.cu

  }

+  TORCH_CHECK(!(std::isinf(minvalue) || std::isinf(maxvalue) || std::isnan(minvalue) || std::isnan(maxvalue)), "range of [", minvalue, ", ", maxvalue, "] is not finite");
+  TORCH_CHECK(minvalue < maxvalue, "max must be larger than min");


Windows CI gives error:

15:26:54 C:\Program Files (x86)\Windows Kits\10\include\10.0.17763.0\ucrt\corecrt_math.h(409): error: more than one instance of overloaded function "fpclassify" matches the argument list: 15:26:54 SummaryOps.cu 15:26:54 function "fpclassify(float)" 15:26:54 function "fpclassify(double)" 15:26:54 function "fpclassify(long double)" 15:26:54 argument types are: (uint8_t) 15:26:54 detected during: 15:26:54 instantiation of "__nv_bool isinf(_Ty) [with _Ty=uint8_t]" 15:26:54 C:/Jenkins/workspace/pytorch-builds/pytorch-win-ws2016-cuda9-cudnn7-py3-build/aten/src/ATen/native/cuda/SummaryOps.cu(326): here 15:26:54 instantiation of "at::Tensor at::<unnamed>::_histc_cuda_template(const at::Tensor &, int64_t, input_t, input_t) [with input_t=uint8_t]" 15:26:54 C:/Jenkins/workspace/pytorch-builds/pytorch-win-ws2016-cuda9-cudnn7-py3-build/aten/src/ATen/native/cuda/SummaryOps.cu(357): here

To fix this issue, in this file I think we should write THCNumerics<input_t>::isinf(val) instead of std::isinf(val), and THCNumerics<input_t>::isnan(val) instead of std::isnan(val) (this is taken from

pytorch/aten/src/ATen/native/cuda/MultinomialKernel.cu

Lines 244 to 245 in 2432986

assert(!THCNumerics<scalar_t>::isinf(val));

assert(!THCNumerics<scalar_t>::isnan(val));

). This only applies to CUDA code though, and we don't need to change the CPU code.

yf225 · 2019-10-18T21:22:51Z

aten/src/ATen/native/cuda/SummaryOps.cu


+  TORCH_CHECK(!(THCNumerics<input_t>::isinf(minvalue) || THCNumerics<input_t>::isinf(maxvalue) || THCNumerics<input_t>::isnan(minvalue) || THCNumerics<input_t>::isnan(maxvalue)), "range of [", minvalue, ", ", maxvalue, "] is not finite");
+  TORCH_CHECK(minvalue < maxvalue, "max must be larger than min");
+


rocm build is failing with:

18:29:27 [ 87%] Linking CXX executable ../bin/generate_proposals_op_gpu_test 18:29:29 ../lib/libtorch.so: undefined reference to `isinf(float)' 18:29:29 ../lib/libtorch.so: undefined reference to `isnan(float)' 18:29:29 ../lib/libtorch.so: undefined reference to `isnan(double)' 18:29:29 ../lib/libtorch.so: undefined reference to `isinf(double)' 18:29:29 clang: �[0;1;31merror: �[0mlinker command failed with exit code 1 (use -v to see invocation)�[0m

To fix this, I think we need to #include <THC/THCNumerics.cuh> at the top of this file after #include <ATen/cuda/CUDAApplyUtils.cuh>, taking inspiration from the aten/src/ATen/native/cuda/MultinomialKernel.cu file.

oh okay 👌 thanks!!

bddppq · 2019-10-21T21:27:58Z

aten/src/TH/generic/THTensorMoreMath.cpp

    maxval = maxval + 1;
  }

+  TORCH_CHECK(!(std::isinf(minval) || std::isinf(maxval) || std::isnan(minval) || std::isnan(maxval)), "range of [", minval, ", ", maxval, "] is not finite");


std::isnan -> th_isnan

yf225 · 2019-10-22T16:27:00Z

Current CI errors seem unrelated and we are ready to merge this PR :D

facebook-github-bot

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

… has inf. also added checks for nan values, min>max (#27712) Summary: pytorch/pytorch#27464 Pull Request resolved: pytorch/pytorch#27712 Differential Revision: D18064544 Pulled By: anjali411 fbshipit-source-id: c9c6d8eb4d55f2b5320409ba238bf44b0be8902e

facebook-github-bot · 2019-10-24T20:40:17Z

@anjali411 merged this pull request in 136bb07.

nairbv · 2019-12-26T16:12:04Z

@anjali411 this previously segfaulted right? so not BC-breaking?

… has inf. also added checks for nan values, min>max (pytorch#27712) Summary: pytorch#27464 Pull Request resolved: pytorch#27712 Differential Revision: D18064544 Pulled By: anjali411 fbshipit-source-id: c9c6d8eb4d55f2b5320409ba238bf44b0be8902e

added a check for finite range

e1b8053

anjali411 requested review from nairbv and yf225 October 10, 2019 18:36

pytorchbot added module: cpu CPU specific problem (e.g., perf, algorithm) module: operators labels Oct 10, 2019

nairbv reviewed Oct 10, 2019

View reviewed changes

nairbv changed the title ~~[histc issue] added a finite range check to resolve torch.histc segfaults if tensor has inf~~ torch.histc added a finite range check to resolve torch.histc segfaults if tensor has inf Oct 10, 2019

nairbv changed the title ~~torch.histc added a finite range check to resolve torch.histc segfaults if tensor has inf~~ torch.histc added a finite range check to resolve torch.histc segfaults if tensor has inf Oct 10, 2019

nairbv changed the title ~~torch.histc added a finite range check to resolve torch.histc segfaults if tensor has inf~~ torch.histc added a finite range check to resolve segfaults if tensor has inf Oct 10, 2019

vadimkantorov reviewed Oct 11, 2019

View reviewed changes

added check for nan and min>max and unit tests

3edac92

anjali411 changed the title ~~torch.histc added a finite range check to resolve segfaults if tensor has inf~~ torch.histc added a finite range check to resolve segfaults if tensor has inf. also added checks for nan values, min>max Oct 11, 2019

nairbv reviewed Oct 11, 2019

View reviewed changes

test/test_torch.py Outdated Show resolved Hide resolved

nairbv reviewed Oct 11, 2019

View reviewed changes

test/test_torch.py Outdated Show resolved Hide resolved

added assertRaisesRegex tests

f874ce9

anjali411 added the module: bc-breaking Related to a BC-breaking change label Oct 14, 2019

anjali411 added 2 commits October 14, 2019 14:11

added finite range checks for histc gpu function

456a9b5

updated var names

8ab4208

anjali411 added the module: cuda Related to torch.cuda, and CUDA support in general label Oct 16, 2019

yf225 approved these changes Oct 18, 2019

View reviewed changes

Merge branch 'master' into histc

3644582

yf225 reviewed Oct 18, 2019

View reviewed changes

windows ci error fix

7e2872a

yf225 reviewed Oct 18, 2019

View reviewed changes

added THC/THCNumerics.cuh in includes

63fa0e6

bddppq reviewed Oct 21, 2019

View reviewed changes

bddppq added 3 commits October 21, 2019 22:10

Merge remote-tracking branch 'origin/master' into histc

0bcd1d8

Use std::{isnan,isinf} in cpu code

38683c9

Use std:: version only in ROCm

4aadb9c

facebook-github-bot reviewed Oct 22, 2019

View reviewed changes

facebook-github-bot closed this in 136bb07 Oct 24, 2019

facebook-github-bot added the merged label Oct 24, 2019

mruberry added the Merged label Oct 28, 2020

		}

		TORCH_CHECK(!(std::isinf(minval) \|\| std::isinf(maxval)), "range of [", minval, ", ", maxval, "] is not finite");

	assert(!THCNumerics<scalar_t>::isinf(val));
	assert(!THCNumerics<scalar_t>::isnan(val));


		TORCH_CHECK(!(THCNumerics<input_t>::isinf(minvalue) \|\| THCNumerics<input_t>::isinf(maxvalue) \|\| THCNumerics<input_t>::isnan(minvalue) \|\| THCNumerics<input_t>::isnan(maxvalue)), "range of [", minvalue, ", ", maxvalue, "] is not finite");
		TORCH_CHECK(minvalue < maxvalue, "max must be larger than min");

torch.histc added a finite range check to resolve segfaults if tensor has inf. also added checks for nan values, min>max #27712

torch.histc added a finite range check to resolve segfaults if tensor has inf. also added checks for nan values, min>max #27712

Uh oh!

Conversation

anjali411 commented Oct 10, 2019

Uh oh!

nairbv Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

vadimkantorov left a comment

Choose a reason for hiding this comment

Uh oh!

anjali411 commented Oct 11, 2019

Uh oh!

Uh oh!

Uh oh!

yf225 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

anjali411 Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

bddppq Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

yf225 commented Oct 22, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 24, 2019

Uh oh!

nairbv commented Dec 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

yf225 left a comment •

edited

Loading