Accept more numpy scalars as doubles #9659

t-vi · 2018-07-20T21:06:21Z

Allows mulitplication of e.g. numpy.float32 with tensors.

This came up with #9468

If you want this and after the other patch is done, I'll add tests (but that would be conflicting, so I prefer to wait).

Allows mulitplication of e.g. numpy.float32 with tensors. This came up with pytorch#9468

soumith · 2018-07-21T04:46:13Z

@pytorchbot retest this please

facebook-github-bot

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

gchanan · 2018-07-22T23:05:12Z

looks good! Maybe add a couple of tests?

t-vi · 2018-07-22T23:11:23Z

I would want to test this by adding numpy float32 scalars to the test introduced in #9651, so my original plan was to revise this PR after that to avoid the merge conflict.

ssnl · 2018-07-23T04:25:58Z

#9651 is BC breaking so this one's probably easier to review. Do you want to add some tests for this first? You can always change the test in #9651 later.

If this is accepted soon, I can cherry-pick this in time into 0.4.1. But I don't think #9651 can make it.

test/test_torch.py

+        for np_dtype in [np.float32, np.float64, np.int32, np.int64]:
+            np_sc = np_dtype(2.0)
+            t = torch.ones(2, requires_grad=True)
+            r1 = t * np_sc


From SsnL's review. Thank you!

t-vi · 2018-07-27T09:23:03Z

@ssnl I added a loop over float/double as input tensor and assert that output is the same dtype. Thanks for the suggestion to check result dtype.
The macos build failure is a network timeout or so, so it doesn't seem to be related to my patch.

facebook-github-bot

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

weiyangfb · 2018-08-13T19:18:24Z

@t-vi needs a rebase now :( Is this ready for merging now? @ssnl @gchanan

yf225 · 2018-08-21T16:56:51Z

@t-vi We probably need to rebase this PR onto current master

t-vi · 2018-08-21T17:29:51Z

Will do next week.

facebook-github-bot

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

weiyangfb · 2018-08-29T04:41:44Z

is this ready to go? @gchanan @ssnl

ssnl · 2018-08-29T05:56:14Z

LGTM. @gchanan any other thoughts?

test/test_torch.py

-        r2 = t * np_sc
-        self.assertIsInstance(r2, torch.Tensor)
-        self.assertTrue(r2.requires_grad)
+        for np_dtype in [np.float32, np.float64, np.int32, np.int64]:


torch/csrc/utils/python_numbers.h

 inline bool THPUtils_checkDouble(PyObject* obj) {
 #if PY_MAJOR_VERSION == 2
-  return PyFloat_Check(obj) || PyLong_Check(obj) || PyInt_Check(obj);
+  return PyFloat_Check(obj) || PyLong_Check(obj) || PyInt_Check(obj) || torch::utils::is_numpy_scalar(obj);


torch/csrc/utils/python_numbers.h

+  return PyFloat_Check(obj) || PyLong_Check(obj) || PyInt_Check(obj) || torch::utils::is_numpy_scalar(obj);
 #else
-  return PyFloat_Check(obj) || PyLong_Check(obj);
+  return PyFloat_Check(obj) || PyLong_Check(obj) || torch::utils::is_numpy_scalar(obj);


And expand range of tested numpy types. Thank you, gchanan for the suggestions!

t-vi · 2018-08-31T13:07:37Z

Is the TestAdam.test_sparse_adam_output_grad caused by the PR? At first sight, I'm not entirely sure I see the relation to the changes.

gchanan · 2018-08-31T17:21:38Z

@pytorchbot retest this please.

gchanan · 2018-08-31T17:21:58Z

I don't think the test failures are relevant; other PRs failed with similar errors around the same time.

facebook-github-bot

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

weiyangfb · 2018-09-05T02:17:42Z

Looks like comments are addressed, should we merge this? @gchanan

gchanan

lgtm.

resolve conflict in data parallel model * master: (201 commits) Add cost inference to ConvGradient and WeightedSum operators (pytorch#10744) Move collapse dims into a single place (pytorch#11272) Fix some more warnings (pytorch#11257) Fix the batchnorm onnx exporting when affine=False Improve error message to include return types too (pytorch#11245) Check doxygen output in travis (pytorch#11124) Accept more numpy scalars as doubles (pytorch#9659) Fixed log message (pytorch#10874) Fix to distribution.__repr__ with lazy attributes (pytorch#11263) Add import export step to end to end tests Add complex hooks for out of tree complex implementation. (pytorch#11216) Unify opt flag for cmake codegen (pytorch#11227) nomnigraph - fix memory error in NN subgraph matchOp (pytorch#11127) Port PackedSequences functions to C++ (pytorch#11224) Treat numerical differences as warnings instead of errors when tracing (pytorch#11246) add a Float16UniformFill (pytorch#11123) Implement torch.tensordot (pytorch#10025) keep net type info when generating model complete net (pytorch#11032) Get rid of some uses of type() (pytorch#11215) Reorganize methods in Type, add CPUTypeDefault/CUDATypeDefault (pytorch#11205) ...

Summary: Allows mulitplication of e.g. numpy.float32 with tensors. This came up with pytorch#9468 If you want this and after the other patch is done, I'll add tests (but that would be conflicting, so I prefer to wait). Pull Request resolved: pytorch#9659 Differential Revision: D8948078 Pulled By: weiyangfb fbshipit-source-id: c7dcc57b63e2f100df837f70e1299395692f1a1b

Accept more numpy scalars as doubles

8ee860e

Allows mulitplication of e.g. numpy.float32 with tensors. This came up with pytorch#9468

t-vi requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners July 20, 2018 21:06

facebook-github-bot reviewed Jul 22, 2018

View reviewed changes

add test with mul

3d841a0

ssnl reviewed Jul 23, 2018

View reviewed changes

test/test_torch.py Outdated

for np_dtype in [np.float32, np.float64, np.int32, np.int64]:

np_sc = np_dtype(2.0)

t = torch.ones(2, requires_grad=True)

r1 = t * np_sc

This comment was marked as off-topic.

Sign in to view

Check for torch dtypes float and double and also assert result dtype

20c2ae3

From SsnL's review. Thank you!

facebook-github-bot reviewed Jul 30, 2018

View reviewed changes

zou3519 added the ready for review (this tag is deprecated) All PRs are ready for review unless they are draft, WIP, or have undismissed requested changes label Jul 31, 2018

weiyangfb mentioned this pull request Aug 13, 2018

[bug] Multiplication of tensor with numpy scalar does not always work #9468

Closed

Merge branch 'master' into accept_more_scalars

c12f116

facebook-github-bot reviewed Aug 29, 2018

View reviewed changes

gchanan reviewed Aug 31, 2018

View reviewed changes

Guard numpy conversion with USE_NUMPY ifdefs

9d0371a

And expand range of tested numpy types. Thank you, gchanan for the suggestions!

Merge branch 'master' into accept_more_scalars

a1d79f1

facebook-github-bot reviewed Sep 4, 2018

View reviewed changes

gchanan approved these changes Sep 5, 2018

View reviewed changes

facebook-github-bot closed this in 267e1ec Sep 5, 2018

ezyang added open source merged labels Jun 24, 2019

Accept more numpy scalars as doubles #9659

Accept more numpy scalars as doubles #9659

Uh oh!

Conversation

t-vi commented Jul 20, 2018

Uh oh!

soumith commented Jul 21, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gchanan commented Jul 22, 2018

Uh oh!

t-vi commented Jul 22, 2018

Uh oh!

ssnl commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

t-vi commented Jul 27, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

weiyangfb commented Aug 13, 2018

Uh oh!

yf225 commented Aug 21, 2018

Uh oh!

t-vi commented Aug 21, 2018 via email

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

weiyangfb commented Aug 29, 2018

Uh oh!

ssnl commented Aug 29, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

t-vi commented Aug 31, 2018

Uh oh!

gchanan commented Aug 31, 2018

Uh oh!

gchanan commented Aug 31, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

weiyangfb commented Sep 5, 2018

Uh oh!

gchanan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ssnl commented Jul 23, 2018 •

edited

Loading