Add amax/amin #43092

zasdfgbnm · 2020-08-15T00:28:04Z

Add a max/min operator that only return values.

Some important decision to discuss

Question	Current State
Expose torch.max_values to python?	No
Remove max_values and only keep amax?	Yes
Should amax support named tensors?	Not in this PR

Numpy compatibility

Reference: https://numpy.org/doc/stable/reference/generated/numpy.amax.html

Parameter	PyTorch Behavior
`axis`: None or int or tuple of ints, optional. Axis or axes along which to operate. By default, flattened input is used. If this is a tuple of ints, the maximum is selected over multiple axes, instead of a single axis or all the axes as before.	Named `dim`, behavior same as `torch.sum` (#29137)
`out`: ndarray, optional. Alternative output array in which to place the result. Must be of the same shape and buffer length as the expected output.	Same
`keepdims`: bool, optional. If this is set to True, the axes which are reduced are left in the result as dimensions with size one. With this option, the result will broadcast correctly against the input array.	implemented as `keepdim`
`initial`: scalar, optional. The minimum value of an output element. Must be present to allow computation on empty slice.	Not implemented in this PR. Better to implement for all reductions in the future.
`where`: array_like of bool, optional. Elements to compare for the maximum.	Not implemented in this PR. Better to implement for all reductions in the future.

Note from numpy:

NaN values are propagated, that is if at least one item is NaN, the corresponding max value will be NaN as well. To ignore NaN values (MATLAB behavior), please use nanmax.

PyTorch has the same behavior

dr-ci · 2020-08-15T02:50:04Z

💊 CI failures summary and remediations

As of commit 422e870 (more details on the Dr. CI page):

2/3 failures introduced in this PR
1/3 tentatively recognized as flaky ❄️
- Click here to rerun these jobs

🕵️ 2 new failures recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

pytorch_windows_vs2019_py36_cuda10.1_test2 (1/2)

Step: "Attaching workspace" (full log | diagnosis details | 🔁 rerun)

Downloading workspace layers

Downloading workspace layers
  workflows/workspaces/68841824-5f4a-4015-b835-20e9672dcb2d/0/0e1964d6-c16b-4de2-bca3-7ee68c84622e/0/105.tar.gz - 8.4 MB
Applying workspace layers
  0e1964d6-c16b-4de2-bca3-7ee68c84622e

pytorch_windows_vs2019_py36_cuda10.1_test1 (2/2)

Step: "Attaching workspace" (full log | diagnosis details | 🔁 rerun)

Downloading workspace layers

Downloading workspace layers
  workflows/workspaces/68841824-5f4a-4015-b835-20e9672dcb2d/0/0e1964d6-c16b-4de2-bca3-7ee68c84622e/0/105.tar.gz - 8.4 MB
Applying workspace layers
  0e1964d6-c16b-4de2-bca3-7ee68c84622e

❄️ 1 failure tentatively classified as flaky

but reruns have not yet been triggered to confirm:

pytorch_linux_xenial_cuda10_2_cudnn7_py3_gcc7_test (1/1)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun) ❄️

Aug 27 09:30:36 ConnectionResetError: [Errno 104] Connection reset by peer

Aug 27 09:30:36   File "/opt/conda/lib/python3.6/multiprocessing/connection.py", line 455, in accept 
Aug 27 09:30:36     deliver_challenge(c, self._authkey) 
Aug 27 09:30:36   File "/opt/conda/lib/python3.6/multiprocessing/connection.py", line 722, in deliver_challenge 
Aug 27 09:30:36     response = connection.recv_bytes(256)        # reject large message 
Aug 27 09:30:36   File "/opt/conda/lib/python3.6/multiprocessing/connection.py", line 216, in recv_bytes 
Aug 27 09:30:36     buf = self._recv_bytes(maxlength) 
Aug 27 09:30:36   File "/opt/conda/lib/python3.6/multiprocessing/connection.py", line 407, in _recv_bytes 
Aug 27 09:30:36     buf = self._recv(4) 
Aug 27 09:30:36   File "/opt/conda/lib/python3.6/multiprocessing/connection.py", line 379, in _recv 
Aug 27 09:30:36     chunk = read(handle, remaining) 
Aug 27 09:30:36 ConnectionResetError: [Errno 104] Connection reset by peer 
Aug 27 09:30:36 /opt/conda/lib/python3.6/multiprocessing/semaphore_tracker.py:143: UserWarning: semaphore_tracker: There appear to be 14 leaked semaphores to clean up at shutdown 
Aug 27 09:30:36   len(cache)) 
Aug 27 09:30:39 Process ErrorTrackingProcess-156: 
Aug 27 09:30:39 Traceback (most recent call last): 
Aug 27 09:30:39   File "/opt/conda/lib/python3.6/multiprocessing/process.py", line 258, in _bootstrap 
Aug 27 09:30:39     self.run() 
Aug 27 09:30:39   File "/var/lib/jenkins/workspace/test/test_dataloader.py", line 361, in run 
Aug 27 09:30:39     super(ErrorTrackingProcess, self).run() 
Aug 27 09:30:39   File "/opt/conda/lib/python3.6/multiprocessing/process.py", line 93, in run 
Aug 27 09:30:39     self._target(*self._args, **self._kwargs)

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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 109 times.

aten/src/ATen/native/native_functions.yaml

mruberry · 2020-08-18T03:27:56Z

Thinking seems good, but PRs introducing new functions typically do add the out variants and derivatives, and they should be straightforward to include.

I do have a question, though: What's the difference between min_values (max_values) and amin (amax)? Should we leave min_values and max_values unchanged and deprecate them?

mruberry · 2020-08-25T23:51:39Z

Overall looking good but you need to update tensors.rst and torch.rst with the new doc entries.

zasdfgbnm · 2020-08-26T00:37:30Z

@mruberry Yes, I missed that point. Added now.

mruberry · 2020-08-26T02:37:06Z

docs/source/torch.rst

@@ -336,6 +336,10 @@ Reduction Ops

    argmax
    argmin
+    amax
+    amin
+    max


This is a better place for max and min but would you please remove them from the "Comparison Ops," too, then? People should be using maximum and minimum, anyway, for comparisons.

mruberry

One last nit about removing the redundant max and min torch.rst entries under Comparison Ops.

Other than that I am satisfied. Let's let @ngimel make her final determination, too.

aten/src/ATen/native/cpu/ReduceOpsKernel.cpp

ngimel · 2020-08-26T17:57:32Z

torch/_torch_docs.py

+           r"""
+amax(input, dim, keepdim=False, *, out=None) -> Tensor
+
+Returns the maximum value of each row of the :attr:`input` tensor in the given


Given that there are potentially multiple dimensions, it's slice, not row

ngimel · 2020-08-26T17:59:39Z

torch/_torch_docs.py

+    The difference between ``max``/``min`` and ``amax``/``amin`` is:
+    1. ``amax``/``amin`` supports reducing on multiple dimensions,
+    2. ``amax``/``amin`` does not return indices,
+    3. ``amax``/``amin`` produces deterministic (sub)gradients unlike


max and min will produce deterministic (but different) gradients soon, so it makes sense to say that amax evenly distributes gradient between equal values, while max propagates gradient only to a single index in the source tensor.

zasdfgbnm · 2020-08-26T21:21:56Z

Just fixed the failures in doc build and doc test.

facebook-github-bot

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

mruberry · 2020-08-27T04:22:45Z

@zasdfgbnm Would you rebase this?

zasdfgbnm · 2020-08-27T05:00:27Z

@mruberry rebased

facebook-github-bot

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

facebook-github-bot · 2020-08-28T20:26:26Z

@mruberry merged this pull request in bcec8cc.

Summary: This is to align with the naming in numpy and in #43092 Test Plan: ``` python test/test_torch.py TestTorchDeviceTypeCPU.test_aminmax_cpu_float32 python test/test_torch.py TestTorchDeviceTypeCUDA.test_aminmax_cuda_float32 ``` Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: This is to align with the naming in numpy and in #43092 Test Plan: ``` python test/test_torch.py TestTorchDeviceTypeCPU.test_aminmax_cpu_float32 python test/test_torch.py TestTorchDeviceTypeCUDA.test_aminmax_cuda_float32 ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D23465298](https://our.internmc.facebook.com/intern/diff/D23465298) [ghstack-poisoned]

Summary: Pull Request resolved: #44001 This is to align with the naming in numpy and in #43092 Test Plan: ``` python test/test_torch.py TestTorchDeviceTypeCPU.test_aminmax_cpu_float32 python test/test_torch.py TestTorchDeviceTypeCUDA.test_aminmax_cuda_float32 ``` Imported from OSS Reviewed By: jerryzh168 Differential Revision: D23465298 fbshipit-source-id: b599035507156cefa53942db05f93242a21c8d06

mruberry · 2020-10-13T18:37:32Z

This was reverted because it triggered a preexisting CUDA bug, relevant log snippet:

test_blas_empty_cuda - TestTorchDeviceTypeCUDA
test_torch.py

Traceback (most recent call last):
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 815, in wrapper
    method(*args, **kwargs)
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 815, in wrapper
    method(*args, **kwargs)
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 257, in instantiated_test
    result = test(self, *args)
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 479, in dep_fn
    return fn(slf, device, *args, **kwargs)
  File "test_torch.py", line 13564, in test_blas_empty
    self.assertEqual(torch.zeros((5, 6), device=device), fn(torch.mm, (5, 0), (0, 6)))
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 1124, in assertEqual
    self.assertTrue(result, msg=msg)
AssertionError: False is not true : Tensors failed to compare as equal! With rtol=1.3e-06 and atol=1e-05, found 10 element(s) (out of 30) whose difference(s) exceeded the margin of error (including 10 nan comparisons). The greatest difference was nan (0.0 vs. nan), which occurred at index (1, 0).

Add amax/amin and max_values and min_values

f582ddf

pytorchbot added the open source label Aug 15, 2020

Merge branch 'master' of github.com:pytorch/pytorch into amaxmin

25b660b

ngimel mentioned this pull request Aug 17, 2020

_min_max_val.dim: CPU implementation #42894

Closed

mruberry reviewed Aug 18, 2020

View reviewed changes

aten/src/ATen/native/native_functions.yaml Outdated Show resolved Hide resolved

mruberry reviewed Aug 18, 2020

View reviewed changes

aten/src/ATen/native/native_functions.yaml Outdated Show resolved Hide resolved

mruberry reviewed Aug 18, 2020

View reviewed changes

aten/src/ATen/native/native_functions.yaml Outdated Show resolved Hide resolved

mruberry reviewed Aug 18, 2020

View reviewed changes

aten/src/ATen/native/native_functions.yaml Outdated Show resolved Hide resolved

zasdfgbnm added 5 commits August 18, 2020 13:37

Remove named tensor support

da2aa2f

Merge branch 'master' of github.com:pytorch/pytorch into amaxmin

b426313

Add .out overload

ef8909d

tests

1698c64

fixes

fc3fc75

zasdfgbnm mentioned this pull request Aug 19, 2020

Vec256<int64_t> does not handle LONG_MAX on minimum #43254

Open

zasdfgbnm added 3 commits August 18, 2020 22:00

fixes

1637b83

more fixes

415052d

Merge branch 'master' of github.com:pytorch/pytorch into amaxmin

fef161e

zasdfgbnm changed the title ~~[WIP]Add amax/amin and max_values and min_values~~ [WIP]Add amax/amin Aug 19, 2020

zasdfgbnm added 2 commits August 19, 2020 16:50

docs

aed1fd7

Merge branch 'master' of github.com:pytorch/pytorch into amaxmin

9a92fd2

This was referenced Aug 20, 2020

Using tuples for torch.max() #31042

Closed

Tensor.max()/min() over multiple axes #28213

Closed

zasdfgbnm added 3 commits August 20, 2020 14:55

grad

1493bfe

Merge branch 'master' of github.com:pytorch/pytorch into amaxmin

5253092

grad

2999719

zasdfgbnm changed the title ~~[WIP]Add amax/amin~~ Add amax/amin Aug 21, 2020

zasdfgbnm marked this pull request as ready for review August 21, 2020 00:33

fix overrides

b437497

mruberry self-requested a review August 25, 2020 23:49

zasdfgbnm added 2 commits August 25, 2020 16:58

docs

dad60d4

Merge branch 'amaxmin' of github.com:pytorch/pytorch into amaxmin

cb4fd5f

mruberry reviewed Aug 26, 2020

View reviewed changes

mruberry approved these changes Aug 26, 2020

View reviewed changes

Update torch.rst

c5adc85

ngimel approved these changes Aug 26, 2020

View reviewed changes

zasdfgbnm added 2 commits August 26, 2020 11:04

Update _torch_docs.py

c2cbbf0

fix ident in docs

11582e5

facebook-github-bot reviewed Aug 26, 2020

View reviewed changes

zasdfgbnm mentioned this pull request Aug 27, 2020

Some largeCUDATensorTest fails with OOM when running with the entire test suit, but not when running standalone #43677

Open

Merge branch 'master' into amaxmin

422e870

facebook-github-bot reviewed Aug 27, 2020

View reviewed changes

facebook-github-bot closed this in bcec8cc Aug 28, 2020

zasdfgbnm deleted the amaxmin branch August 28, 2020 20:06

facebook-github-bot added the merged label Aug 28, 2020

vkuzo mentioned this pull request Sep 2, 2020

rename _min_max to _aminmax #44001

Closed

mruberry added the Merged label Oct 28, 2020

pyscorcher mentioned this pull request Jul 9, 2021

Reduce with any(), all(), median() over multiple dimensions #61453

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add amax/amin #43092

Add amax/amin #43092

zasdfgbnm commented Aug 15, 2020 •

edited

dr-ci bot commented Aug 15, 2020 •

edited

mruberry commented Aug 18, 2020

mruberry commented Aug 25, 2020

zasdfgbnm commented Aug 26, 2020

mruberry Aug 26, 2020

zasdfgbnm Aug 26, 2020

mruberry left a comment

ngimel Aug 26, 2020

zasdfgbnm Aug 26, 2020

ngimel Aug 26, 2020

zasdfgbnm Aug 26, 2020

zasdfgbnm commented Aug 26, 2020

facebook-github-bot left a comment

mruberry commented Aug 27, 2020

zasdfgbnm commented Aug 27, 2020

facebook-github-bot left a comment

facebook-github-bot commented Aug 28, 2020

mruberry commented Oct 13, 2020

@@ @@ -336,6 +336,10 @@ Reduction Ops @@
                   argmax
                   argmin
+                  amax
+                  amin
+                  max

Add amax/amin #43092

Add amax/amin #43092

Conversation

zasdfgbnm commented Aug 15, 2020 • edited

Some important decision to discuss

Numpy compatibility

dr-ci bot commented Aug 15, 2020 • edited

💊 CI failures summary and remediations

🕵️ 2 new failures recognized by patterns

pytorch_windows_vs2019_py36_cuda10.1_test2 (1/2)

pytorch_windows_vs2019_py36_cuda10.1_test1 (2/2)

❄️ 1 failure tentatively classified as flaky

pytorch_linux_xenial_cuda10_2_cudnn7_py3_gcc7_test (1/1)

mruberry commented Aug 18, 2020

mruberry commented Aug 25, 2020

zasdfgbnm commented Aug 26, 2020

mruberry Aug 26, 2020

Choose a reason for hiding this comment

zasdfgbnm Aug 26, 2020

Choose a reason for hiding this comment

mruberry left a comment

Choose a reason for hiding this comment

ngimel Aug 26, 2020

Choose a reason for hiding this comment

zasdfgbnm Aug 26, 2020

Choose a reason for hiding this comment

ngimel Aug 26, 2020

Choose a reason for hiding this comment

zasdfgbnm Aug 26, 2020

Choose a reason for hiding this comment

zasdfgbnm commented Aug 26, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

mruberry commented Aug 27, 2020

zasdfgbnm commented Aug 27, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 28, 2020

mruberry commented Oct 13, 2020

zasdfgbnm commented Aug 15, 2020 •

edited

dr-ci bot commented Aug 15, 2020 •

edited