Resolving comments from Bool Tensor for CPU PR (#18165)
authorIurii Zdebskyi <iuriiz@fb.com>
Tue, 26 Mar 2019 16:55:50 +0000 (09:55 -0700)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Tue, 26 Mar 2019 16:59:34 +0000 (09:59 -0700)
commit1a742075ee97b9603001188eeec9c30c3fe8a161
tree3a6878a10465718be27079d3c24eeac68e75f960
parent515238e0a54f1a00a67c984dd65d906acdc57034
Resolving comments from Bool Tensor for CPU PR (#18165)

Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/18165
ghimport-source-id: 55cb3fb63a25c2faab1725b4ec14c688bf45bd38

Stack from [ghstack](https://github.com/ezyang/ghstack):
* #18166 Bool Tensor for CUDA
* **#18165 Resolved comments from Bool Tensor for CPU PR**
-------
------------
This is a follow up PR that resolves some additional feedback on one the of previous Bool Tensor PRs.

gchanan, here is a list of almost all the comments from the original PR with respective fixes and replies:

**[utils/python_scalars.h]** why is this converting from uint8_t and not bool? (comment?)
When i was adding this, i was testing by creating a tensor and then calling its .tolist(). it worked for bool and uint8_t equally good so i left uint8_t as thought it makes more sense as we are calling PyBool_FromLong. �Changing it to bool.

**[ATen/Dispatch.h]**better name?.
fixed.

**[test/test_torch.py]** what about other factories, such as full? (and more).
There is a test that goes through the factory methods - test_tensor_factories_empty. i added some bool cases above it and added a comment that once CUDA will be done, i will unite them and it will iterate not just between CUDA and CPU but also all types. ��Adding all bool cases now. Will unite in CUDA PR.

**[generic/THTensorMath.h]** any changes in this file actually needed?
Bad merge. Fixed.

**[TH/THTensor.h]** this generates code for random, clampedRandom, and cappedRandom -- do we have tests for all of these with bool?
Added

**[c10/core/ScalarType.h]** I'm not very confident about the lack of Bool here -- can you look at the call sites and see what makes sense to do here?
Added bool to the macro and created a similar one without for a single case which fails the build with errors:

_./torch/csrc/jit/symbolic_variable.h:79:20: error: ambiguous overload for ‘operator*’ (operand types are ‘const torch::jit::SymbolicVariable’ and ‘torch::jit::Value*’)
return (*this) * insertConstant(rhs);_

Differential Revision: D14605105

fbshipit-source-id: abf82d50e8f8c50b386545ac068268651b28496d
12 files changed:
aten/src/ATen/Dispatch.h
aten/src/ATen/native/Copy.cpp
aten/src/ATen/native/TensorFactories.cpp
aten/src/TH/generic/THTensorMath.cpp
aten/src/TH/generic/THTensorMath.h
c10/core/Scalar.h
c10/core/ScalarType.h
caffe2/contrib/aten/aten_op_template.h
caffe2/operators/experimental/c10/cpu/cast_cpu.cc
test/common_utils.py
test/test_torch.py
torch/csrc/utils/python_scalars.h