Skip to content

Conversation

@t-vi
Copy link
Collaborator

@t-vi t-vi commented Jul 20, 2018

This causes numpy to yield to the torch functions,
e.g. instead of numpy array/scalar mul converting the tensor to
an array, it will now arrange for the Tensor rmul to be called.

Fixes case 2 of #9468
I also makes case 3 and 4 equivalent but does not fix them.

This causes numpy to yield to the torch functions,
e.g. instead of numpy array/scalar __mul__ converting the tensor to
an array, it will now arrange for the Tensor __rmul__ to be called.

Fixes case 2 of pytorch#9468
I also makes case 3 and 4 equivalent but does not fix them.
@t-vi
Copy link
Collaborator Author

t-vi commented Jul 20, 2018

Ha, so some of the tests depended on the old behaviour of silently making array of things. I'll fix the tests...

@soumith
Copy link
Contributor

soumith commented Jul 21, 2018

@pytorchbot retest this please

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.

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

@t-vi
Copy link
Collaborator Author

t-vi commented Jul 23, 2018

A much more elaborate way might be to make the array priority of new tensors configurable (easy) and possibly even give calculation results the max of the inputs (quite invasive).

Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

I don't really know what __array_priority__ etiquette is, but this totally seems reasonable.

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.

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

@ssnl
Copy link
Collaborator

ssnl commented Jul 23, 2018 via email

@ezyang
Copy link
Contributor

ezyang commented Jul 23, 2018

Thanks for the note @ssnl. I'll unapprove for now until I understand it better.

Ha, so some of the tests depended on the old behaviour of silently making array of things.

@t-vi Do you know exactly what happened here?

ezyang
ezyang previously requested changes Jul 23, 2018
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

need to work out BC considerations

@t-vi
Copy link
Collaborator Author

t-vi commented Jul 23, 2018

So we passed a tensor to scipy.stat.binom.
Previously, that would be cast to an array whenever a binary operator with an array was used.
Now things become tensors in these cases - except when we define the op, as is the case for "&".
So some check failed because it passed an array to scipy and that tried "array & tensor".

Now if you look at https://github.com/scipy/scipy/blob/master/scipy/stats/_distn_infrastructure.py, you see that most functions use asarray on their inputs. Apparently we managed to hit one that doesn't. However, it seems that that not using asarray is considered a scipy bug - the latest commit is from June and changes that for a particular instance with a comment indicating that it is considered a bugfix.

So in the the spectrum of "mixing non-tensor and tensor" you could have these options

  • depend on who's left and right of a binary op (current behaviour - but highly inconsistent),
  • always yield to the other, (setting a strongly negative array_priority),
  • return a tensor for scalar + tensor but an array for array + tensor (setting a slightly negative array priority),
  • always return a tensor (setting a positive array priority - this is what this patch does).

That means that functions who didn't use asarray and relied on some binary op doing an implicit conversion and actually need an array now fail.

So in summary, I'm not terribly worried about the backwards compatibility - it was more than fragile before - if scipy decided to do a * b instead of b * a somewhere, code would suddenly fail. In fact, I think any consistent behaviour would be better, setting a positive array_priority just is PyTorch vanity. ;)

@ezyang ezyang dismissed their stale review July 30, 2018 21:16

I'm convinced

jramseyer pushed a commit to jramseyer/pytorch that referenced this pull request Jul 31, 2018
Summary:
This causes numpy to yield to the torch functions,
e.g. instead of numpy array/scalar __mul__ converting the tensor to
an array, it will now arrange for the Tensor __rmul__ to be called.

Fixes case 2 of pytorch#9468
I also makes case 3 and 4 equivalent but does not fix them.
Pull Request resolved: pytorch#9651

Differential Revision: D8948079

Pulled By: ezyang

fbshipit-source-id: bd42c04e96783da0bd340f37f4ac3559e9bbf8db
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
Summary:
This causes numpy to yield to the torch functions,
e.g. instead of numpy array/scalar __mul__ converting the tensor to
an array, it will now arrange for the Tensor __rmul__ to be called.

Fixes case 2 of pytorch#9468
I also makes case 3 and 4 equivalent but does not fix them.
Pull Request resolved: pytorch#9651

Differential Revision: D8948079

Pulled By: ezyang

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants