Add support of variadic length/type argument in `result_type` #61168

asi1024 · 2021-07-02T07:24:41Z

Fixes #51284 (cc/ @mruberry, @heitorschueroff, @rgommers, @emcastillo, @kmaehashi)

This PR adds support of variadic length/type argument in torch.result_type for the compatibility with NumPy’s interface and Python array API standard.

>>> torch.result_type(torch.int8, torch.tensor([1], dtype=torch.uint8), 10)
torch.int16

Reference: #51284 (comment)

TODO:

tests

facebook-github-bot · 2021-07-02T07:24:47Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/61168
📄 Preview docs built from this PR
📄 Preview C++ docs built from this PR
🔧 Opt-in to CIFlow to control what jobs run on your PRs

💊 CI failures summary and remediations

As of commit 9d53b03 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

torch/functional.py

rgommers · 2021-07-04T08:21:06Z

Thanks @asi1024!

A Windows vmap test seems unhappy:

RuntimeError: Batching rule not implemented for aten::result_type.TensorList. We could not generate a fallback.

asi1024 · 2021-07-05T10:43:18Z

@rgommers I found an unexpected behavior in numpy.result_type (perhaps it is a bug in numpy.result_type?)

>>> numpy.result_type(numpy.int8, True)
dtype('int8')
>>> numpy.result_type(numpy.int8, 10)
dtype('int8')
>>> numpy.result_type(numpy.int8, True, 10)
dtype('int16')

torch.result_type should return int16 for (numpy.int8, True, 10) input, or int8?

mruberry · 2021-07-07T17:48:09Z

@rgommers I found an unexpected behavior in numpy.result_type (perhaps it is a bug in numpy.result_type?)
>>> numpy.result_type(numpy.int8, True)
dtype('int8')
>>> numpy.result_type(numpy.int8, 10)
dtype('int8')
>>> numpy.result_type(numpy.int8, True, 10)
dtype('int16')
torch.result_type should return int16 for (numpy.int8, True, 10) input, or int8?

I think we should return int8 in this case. @rgommers?

asi1024 · 2021-07-19T08:24:24Z

@rgommers Could you help me how to resolve the vmap and mypy CI failures?

asi1024 · 2021-07-20T15:02:42Z

@mruberry @rgommers Now all tests have passed! Could you take another look?

mruberry · 2021-07-21T09:00:22Z

test/test_type_promotion.py

+            inputs = [_convert_to_numpy_input(x) for x in inputs]
+            return np.result_type(*inputs)
+
+        for x1 in inputs:


Style nit:

for x0, x1, x2 in product(inputs * 3):

torch/functional.py

mruberry · 2021-07-21T09:22:20Z

test/test_type_promotion.py

+        # if inputs of mixed dtypes are given. The tests for floating type inputs are left to
+        # `test_result_type` and `test_result_type_tensor_vs_scalar`.
+        dtypes = [torch.bool, torch.uint8, torch.int16, torch.int64]
+        inputs = [


Instead of comparing with NumPy what about implementing the reference using the addition operator?

def result_type_ref(args): assert len(args) > 0 t = torch.tensor(True) for arg in args: if isinstance(arg, torch.dtype): # See comment below for a discussion of what type of object dtype should be represented by t = t + torch.tensor((1,), dtype=arg) else: t = t + arg return t.dtype

I think this would allow for a simpler test structure and more thorough testing, plus it would compare with the ground truth of what PyTorch is doing

I think the reference implementation using + sometimes returns unexpected results.

>>> a = torch.tensor([1], dtype=torch.int8) >>> b = 1. >>> c = torch.tensor([1], dtype=torch.float16) >>> (a + b + c).dtype torch.float32 >>> (a + c + b).dtype torch.float16

Do you have some ideas for this issue?

That's really cool!

And it's true, the where our dtypes work the addition operator isn't commutative. Both JAX and NumPy's result_type appears to handle this case correctly.

So I think my suggestion was mistaken. Sorry about that @asi1024. I didn't realize that result_type() can't be thought of as a series of binary elementwise operations. You are right and we can't use this as a reference implementation.

Returning to the previous tests could be a reasonable solution. If we want to extend that further we could add some "golden value" tests with manually generated checks.

After I thought further, I have an idea for a reference implementation.

def result_type_ref(args): assert len(args) > 0 tensor = torch.tensor([True]) tensor_0dim = torch.tensor(True) scalar = True for arg in args: if isinstance(arg, torch.dtype): tensor = tensor + torch.tensor([1], dtype=arg) elif isinstance(arg, torch.Tensor): if arg.ndim == 0: tensor_0dim = tensor_0dim + arg else: tensor = tensor + arg elif isinstance(arg, Number): scalar = scalar + arg else: assert False, "unknown type" return ((tensor + tensor_0dim) + scalar).dtype

It just uses the same logic as the result_type implementation in this PR. Can I use this implementation in my tests?

I think you're right, that's a clever approach, @asi1024. Let's try using this

mruberry

Overall this looks great to me, @asi1024; I made a couple inline comments for your review

cc @heitorschueroff -- would you take a look at the functional implementation?
cc @bhosmer -- would you or someone else from the composability team like to look at the dispatch extension?

ezyang · 2021-07-22T16:00:12Z

the python arg parser changes look fine

heitorschueroff · 2021-07-23T17:48:44Z

test/test_type_promotion.py

@@ -402,6 +402,39 @@ def _test_spot(a, b, res_dtype):
                   torch.tensor(1., dtype=torch.complex64, device=device), torch.complex128)
        _test_spot(torch.tensor([1, 1], dtype=torch.bool, device=device), 1., torch.get_default_dtype())

+    @unittest.skipIf(not TEST_NUMPY, "NumPy not found")


@mruberry Is this skip necessary? I thought we always include NumPy now.

Correct. We require NumPy as a dependency when running the test suite

heitorschueroff

Overall this PR looks great! Besides a few nit comments, the main thing we need to discuss is the behavior of scalar tensors. I think we should follow the array api standard and treat scalar tensors (0 dimensions) the same as every other tensor. And treat tensors with higher priority than python scalars. If this is the case, I think the logic in functional.py has to change a bit. @mruberry and @rgommers what do you think?

Thanks for this excellent PR.

heitorschueroff · 2021-07-23T17:51:08Z

test/test_type_promotion.py

+                if isinstance(x, int):
+                    return np.int64(x)
+                return torch.tensor([1], dtype=x).numpy().dtype
+            inputs = [_convert_to_numpy_input(x) for x in inputs]


nit: writing this as a regular for loop instead of defining a method just to be able to use a list comprehension is more readable.

torch/csrc/utils/python_arg_parser.cpp

heitorschueroff · 2021-07-23T17:57:15Z

torch/functional.py

@@ -1589,3 +1590,49 @@ def _lu_no_infos(A, pivot=True, get_infos=False, out=None):

 def align_tensors(*tensors):
    raise RuntimeError('`align_tensors` not yet implemented.')
+
+def result_type(*arrays_and_dtypes: Union[Tensor, torch.dtype, bool, int, float, complex]) -> torch.dtype:


complex can be used to represent any complex, float and int in Python typing (see https://www.python.org/dev/peps/pep-0484/#the-numeric-tower).

More generically there's Number (https://docs.python.org/3/library/numbers.html), but this seems fine

heitorschueroff · 2021-07-23T18:00:12Z

torch/functional.py

@@ -1589,3 +1590,49 @@ def _lu_no_infos(A, pivot=True, get_infos=False, out=None):

 def align_tensors(*tensors):
    raise RuntimeError('`align_tensors` not yet implemented.')
+
+def result_type(*arrays_and_dtypes: Union[Tensor, torch.dtype, bool, int, float, complex]) -> torch.dtype:
+    """result_type(*arrays_and_dtypes: Union[Tensor, dtype, bool, int, float, complex]) -> dtype


I believe the python array API standard mentions leaving out type annotations from signature and only including them in the parameter list (https://data-apis.org/array-api/latest/API_specification/data_type_functions.html?highlight=result_type#result-type-arrays-and-dtypes).

torch/functional.py

heitorschueroff · 2021-07-23T18:17:46Z

torch/functional.py

+    >>> torch.result_type(torch.tensor([1, 2], dtype=torch.uint8), torch.tensor(1))
+    torch.uint8
+    >>> torch.result_type(torch.int32, torch.float32)
+    torch.float32


an example including tensor, scalar and dtype?

heitorschueroff · 2021-07-23T18:20:02Z

torch/functional.py

+    tensors = []
+    scalars: List[Union[bool, int, float, complex]] = []
+    dtypes = []


nit: type annotation for the other lists

heitorschueroff · 2021-07-23T18:20:35Z

torch/functional.py

+            scalars.append(x)
+        elif isinstance(x, torch.dtype):
+            dtypes.append(x)
+        else:


change to elif isinstance(x, torch.Tensor) and add an else clause and raise an Error

heitorschueroff · 2021-07-23T18:23:52Z

torch/functional.py

+        if dtypes:
+            return _VF._result_type_dtypes(dtypes)
+        else:
+            raise TypeError("at least one argument is required.")


change message to result_type(): must provide at least one argument

torch/functional.py

heitorschueroff · 2021-07-23T18:44:05Z

On second thought, if we are giving higher priority to tensors over python scalars such that if there is at least one tensor, python scalars do no affect the result. Then is there a use case for wanting to know the result_type between python scalars? such as torch.result_type(1, 2.0)? The parameter name itself says arrays_and_dtypes. I think that unless there has been request for it, we could drop the support for python scalars and that would also simplify the logic. @mruberry

mruberry · 2021-07-24T10:25:25Z

On second thought, if we are giving higher priority to tensors over python scalars such that if there is at least one tensor, python scalars do no affect the result.

Scalars do affect the choice of computation dtype because the scalar might have a higher type kind than the tensor. For example when adding a float tensor to an int tensor the result is a float tensor.

Then is there a use case for wanting to know the result_type between python scalars? such as torch.result_type(1, 2.0)? The parameter name itself says arrays_and_dtypes. I think that unless there has been request for it, we could drop the support for python scalars and that would also simplify the logic. @mruberry

Most PyTorch operations don't support exclusively scalar arguments but some do and it's a nice feature to support.

mruberry · 2021-07-30T04:47:27Z

Overall this PR looks great! Besides a few nit comments, the main thing we need to discuss is the behavior of scalar tensors. I think we should follow the array api standard and treat scalar tensors (0 dimensions) the same as every other tensor. And treat tensors with higher priority than python scalars. If this is the case, I think the logic in functional.py has to change a bit. @mruberry and @rgommers what do you think?

Thanks for this excellent PR.

We should do this but we don't today, so this PR correctly implements PyTorch's current behavior.

mruberry

Hey @asi1024! Thank you for your thoughtful responses. There are a few minor comments still inline waiting for your review, but per your analysis this look good overall.

Would you make a last pass and then ping me when you're happy to merge this?

asi1024 · 2021-08-03T17:27:14Z

@mruberry
I found a bug in my result_type implementation and fixed it in 74bfae6.
CIs have passed excepts for one timeout. Could you take another look? 😃

mruberry · 2021-08-17T05:52:25Z

torch/functional.py

+            tensors.append(x)
+        else:
+            raise TypeError(f"result_type(): cannot interpret '{x}' as a data type")
+    if dtypes:


Check the length of these lists explicitly instead of just using if on them

mruberry · 2021-08-17T06:08:48Z

test/test_type_promotion.py

+            10,  # int scalar
+            10.0,  # float scalar
+            10j,  # complex scalar
+            *[torch.tensor([1, 2, 3], dtype=dtype) for dtype in dtypes],  # tensors


This test is always running on the CPU because it's ignoring the device arg.

Does the current implementation work with CUDA tensors? What if both CUDA and CPU tensors are passed?

I added tests for CUDA/mixed device tensors and confirmed that the current implementation passes the tests!

mruberry · 2021-08-17T06:09:29Z

torch/functional.py

+            raise TypeError(f"result_type(): cannot interpret '{x}' as a data type")
+    if dtypes:
+        dtype = _VF._result_type_dtypes(dtypes)
+        tensors.append(torch.tensor([], dtype=dtype))


Modeling this as creating a tensor seems a little odd and I'm guessing this won't work if calling result_type() with CUDA tensors (currently CUDA inputs are not tested, see above comment).

mruberry · 2021-08-17T06:10:01Z

torch/functional.py

+        scalar_dtype = _VF._result_type_scalars(scalars)  # type: ignore[arg-type]
+        if tensors:
+            tensor_dtype = _VF.result_type(tensors)  # type: ignore[attr-defined]
+            return _VF.result_type(_VF.tensor([], dtype=tensor_dtype),  # type: ignore[attr-defined]


At this point it might be preferable to perform an addition like in the reference implementation

mruberry · 2021-08-17T06:16:33Z

torch/functional.py

+    torch.float32
+"""
+
+    tensors: List[torch.Tensor] = []


Possible alternative implementation of this, inspired by your proposed reference implementation above:

assert len(arrays_and_dtypes) > 0 tensors = [t for t in arrays_and_dtypes if isinstance(t, torch.Tensor)] scalars = [s for s in arrays_and_dtypes if isinstance(t, Number)] dtypes = [d for d in arrays_and_dtypes if isinstance(t, torch.dtype)] tensor_dtype = None if len(tensors) == 0 else _VF._result_type(tensors) scalar_dtype = None if len(scalars) == 0 else _VF._result_type_scalars(scalars) dtype_dtype = None if len(dtypes) == 0 else _VF._result_type_dtypes(dtypes) return (torch.tensor([], dtype=tensor_dtype) + torch.tensor([], dtype=dtype_dtype) + torch.tensor(0, dtype=scalar_dtype)).dtype

Follow-up question: will this or an implementation like the current version work with the jit scripting and tracing? Is there a test for that?

If the jit is still an issue let me know ASAP and I can help modify the PR to fix that.

I have confirmed that the jit scripting/tracing test works fine locally, but I have not added tests yet. In which file should I add the tests?

test_jit.py

mruberry

Sorry again for the wait, @asi1024. I made some inline comments. Your suggestion for a new reference implementation looks very good. Let me know if you have any issues, especially issues with scripting or tracing. If you do we can help modify the PR. Your logic is the important part.

asi1024 · 2021-09-17T10:53:15Z

@mruberry Sorry I totally missed your review comments for long days 🙇
I confirmed that the current implementation works also with CUDA device tensors, but is it still preferable to rewrite it like the reference implementation?

codecov · 2021-09-17T15:56:30Z

Codecov Report

Merging #61168 (9d53b03) into master (f69cf3c) will decrease coverage by 0.10%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##           master   #61168      +/-   ##
==========================================
- Coverage   66.38%   66.28%   -0.11%     
==========================================
  Files         727      734       +7     
  Lines       93573    94079     +506     
==========================================
+ Hits        62117    62357     +240     
- Misses      31456    31722     +266

mruberry · 2021-10-14T04:41:01Z

@mruberry Sorry I totally missed your review comments for long days 🙇 I confirmed that the current implementation works also with CUDA device tensors, but is it still preferable to rewrite it like the reference implementation?

And I missed your update! Sorry @asi1024! I'll take another look at the PR now. I don't know the answer to your question offhand.

mruberry

Had a chance to read through and this looks pretty good, @asi1024! One of our new colleagues, @saketh-are, just started investigating PyTorch's type promotion and I'd like him to take a look, too.

saketh-are · 2021-10-25T04:11:42Z

@asi1024 I had a chance to look through this and it looks good to me.

Just FYI I am working on a type promotion change after which 0-dim tensor operands and dimensioned tensor operands will be treated identically. I don't think this PR needs to change anywhere, though, assuming it's merged in first.

facebook-github-bot · 2021-10-26T06:42:28Z

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

mruberry · 2021-10-29T16:03:36Z

Unfortunately it looks like this is hitting an internal merge failure that will need further review. Sorry for the delay, @asi1024.

github-actions · 2022-04-13T09:42:11Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

asi1024 requested a review from ezyang as a code owner July 2, 2021 07:24

facebook-github-bot added the cla signed label Jul 2, 2021

pytorchbot added the open source label Jul 2, 2021

heitorschueroff self-requested a review July 2, 2021 14:19

rgommers reviewed Jul 4, 2021

View reviewed changes

torch/functional.py Show resolved Hide resolved

ezyang removed their request for review July 6, 2021 14:06

iramazanli added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 8, 2021

asi1024 force-pushed the result_type branch from 9aacce2 to 5173838 Compare July 20, 2021 11:07

mruberry reviewed Jul 21, 2021

View reviewed changes

torch/functional.py Show resolved Hide resolved

mruberry reviewed Jul 21, 2021

View reviewed changes

ezyang self-requested a review July 22, 2021 14:04

ezyang closed this Jul 22, 2021

ezyang reopened this Jul 22, 2021

heitorschueroff reviewed Jul 23, 2021

View reviewed changes

mruberry reviewed Jul 30, 2021

View reviewed changes

asi1024 force-pushed the result_type branch from d214f98 to b36a878 Compare August 3, 2021 10:30

mruberry reviewed Aug 17, 2021

View reviewed changes

asi1024 added 11 commits September 16, 2021 09:53

Add support of ScalarType[]

95d348c

Support result_type for ScalarList and ScalarTypeList

db5fb19

Add result_type implementation in functional.py

3be81a4

Add result_type tests for variadic types input

90fd682

Update result_type documentation

f6cc3f6

Fix mypy failures

7b11b56

Fix vmap test failure

2a9c023

Fix result_type implementation

cd79543

Fix result_type test with reference function

0cecefa

cosmetics

70c8d36

Add result_type tests for mixed device

9d53b03

asi1024 force-pushed the result_type branch from b36a878 to 9d53b03 Compare September 17, 2021 09:50

mruberry reviewed Oct 14, 2021

View reviewed changes

github-actions bot added the Stale label Apr 13, 2022

rgommers added the module: python array api Issues related to the Python Array API label Apr 13, 2022

github-actions bot closed this May 13, 2022

AnirudhDagar mentioned this pull request Apr 13, 2023

RFC: SciPy array types & libraries support scipy/scipy#18286

Open

Add support of variadic length/type argument in result_type #61168

Add support of variadic length/type argument in result_type #61168

Conversation

asi1024 commented Jul 2, 2021 • edited

facebook-github-bot commented Jul 2, 2021 • edited

🔗 Helpful links

💊 CI failures summary and remediations

rgommers commented Jul 4, 2021

asi1024 commented Jul 5, 2021 • edited

mruberry commented Jul 7, 2021

asi1024 commented Jul 19, 2021

asi1024 commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mruberry left a comment

Choose a reason for hiding this comment

ezyang commented Jul 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heitorschueroff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heitorschueroff commented Jul 23, 2021

mruberry commented Jul 24, 2021

mruberry commented Jul 30, 2021

mruberry left a comment

Choose a reason for hiding this comment

asi1024 commented Aug 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mruberry left a comment

Choose a reason for hiding this comment

asi1024 commented Sep 17, 2021

codecov bot commented Sep 17, 2021

Codecov Report

mruberry commented Oct 14, 2021

mruberry left a comment

Choose a reason for hiding this comment

saketh-are commented Oct 25, 2021

facebook-github-bot commented Oct 26, 2021

mruberry commented Oct 29, 2021

github-actions bot commented Apr 13, 2022

Add support of variadic length/type argument in `result_type` #61168

Add support of variadic length/type argument in `result_type` #61168

asi1024 commented Jul 2, 2021 •

edited

facebook-github-bot commented Jul 2, 2021 •

edited

asi1024 commented Jul 5, 2021 •

edited