[primtorch] add reference for clamp_min/clamp_max #79821

jjsjann123 · 2022-06-17T22:46:22Z

Added reference implementation for the two ops;
Added opinfo tests for aten clamp_min/clamp_max;
Added opinfo reference test.

facebook-github-bot · 2022-06-17T22:46:29Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/79821
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

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

Expand to see more

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

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

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

Click here to manually regenerate this comment.

jjsjann123 · 2022-06-17T22:54:24Z

torch/testing/_internal/common_methods_invocations.py

+        torch_opinfo_name="clamp_min",
+        supports_nvfuser=False,
+        skips=(
+            DecorateInfo(unittest.expectedFailure, 'TestCommon', 'test_python_ref_errors'),


The same expectedFailure is under _refs.minimum & _refs.maximum, which doesn't have a comment explaining why.

Since our ref uses those as well, should be the same root cause.

OK but what is the root cause? (Although this is probably moot because of the proposed switch to where above)

ngimel · 2022-06-17T23:34:50Z

torch/_refs/__init__.py

+) -> TensorLikeType:
+    self, min = _maybe_broadcast(self, min)
+
+    return maximum(self, min)


to preserve gradient behavior (clamp doesn't spread gradients when boundary and input are the same) it's better to use where

Good catch! So we should also change clamp to where as well.

clamp doesn't spread gradients when boundary and input are the same

I think clamp does propagate gradient when input/bound equals. But the grad behavior is indeed different from minimum/maximum

In [31]: x = torch.ones(2, 2).requires_grad_() In [32]: torch.clamp_max(x, torch.tensor(1.0)).sum().backward() In [33]: x.grad Out[33]: tensor([[1., 1.], [1., 1.]]) In [34]: x = torch.ones(2, 2).requires_grad_() In [35]: torch.minimum(x, torch.tensor(1.0)).sum().backward() In [36]: x.grad Out[36]: tensor([[0.5000, 0.5000], [0.5000, 0.5000]])

mruberry · 2022-06-17T23:51:41Z

torch/testing/_internal/common_methods_invocations.py

        yield SampleInput(a, args=(b, c))


+def _clamp_min_numpy(a, min=None):


Nice references

mruberry · 2022-06-17T23:51:49Z

torch/testing/_internal/common_methods_invocations.py

+                    skips=(
+                        DecorateInfo(unittest.skip('Skipped!'), 'TestCommon', 'test_dtypes'),
+                    )),
+    BinaryUfuncInfo('clamp_min',


Great OpInfo additions

mruberry · 2022-06-17T23:53:13Z

torch/_refs/__init__.py

+    type_promoting_args=("self", "min"),
+    type_promotion_kind=ELEMENTWISE_TYPE_PROMOTION_KIND.DEFAULT,
+)
+def clamp_min(


Should these use elementwise binary helper?

pytorch/torch/_refs/__init__.py

Line 684 in 399b3dc

def _make_elementwise_binary_reference(

I think it would take care of out and type promotion and _maybe_broadcast?

ngimel · 2022-06-23T18:54:02Z

Yeah when there are 2 tensors, you can't have a single where and still reproduce correct nan propagation behavior, it requires a couple where's or maybe where and isnan

jjsjann123 · 2022-06-23T21:00:48Z

Yeah when there are 2 tensors, you can't have a single where and still reproduce correct nan propagation behavior, it requires a couple where's or maybe where and isnan

Recording some offline discussion for my own sake.

we want correct nan propagation. I need to fix that.
we want extremal propagation properly tested, which is not in current opinfo tests I put there. (otherwise it should fail). double check that.
correct grad backward behavior is just nice-to-have. don't worry too much about that. (since short term backward is traced at torch level, so we'll have clamp_xxx_backward). Having said that, we also shouldn't fallback to maximum/minimum implementation.

jjsjann123 · 2022-06-24T19:29:34Z

Yeah when there are 2 tensors, you can't have a single where and still reproduce correct nan propagation behavior, it requires a couple where's or maybe where and isnan

I was dumb and forgot that nan propagation on forward has already been fixed few days ago. Did a quick refactor to remove redundant where/isnan. verified the test in opinfo for nan propagation. Put a note there on possible breaking gradient behavior.

This PR should be good to go for now. @ngimel @mruberry

jjsjann123 · 2022-06-24T22:25:43Z

torch/_refs/__init__.py

    if min is not None:
-        return maximum(a, min)
+        a_isnan = isnan(a)
+        condition = bitwise_or(ge(a, min), a_isnan)


Highlight this section for nan propagation. Tagging @ngimel

Yeah looks correct

mruberry · 2022-06-27T14:48:27Z

torch/testing/_internal/common_methods_invocations.py

+        supports_nvfuser=False,
+        skips=(
+            # test error disabled since rhs non-tensor python scalar is supported
+            DecorateInfo(unittest.skip("Skipped!"), 'TestCommon', 'test_python_ref_errors'),


Convert these skips to xfails so when the issue is fixed we know to enable the test

mruberry

Cool -- but swap the skips for xfails

mruberry · 2022-06-28T17:47:31Z

torch/testing/_internal/common_methods_invocations.py

+                        DecorateInfo(unittest.expectedFailure,
+                                     'TestBinaryUfuncs',
+                                     'test_type_promotion',
+                                     device_type='cuda'),


Should this just xfail on the complex dtypes?

Oh, no... that test isn't instantiated for multiple dtypes, I think. My mistake.

No worries. There's a single test there test_type_promotion_clamp_max_cuda (__main__.TestBinaryUfuncsCUDA) ... expected failure.

I checked a few other places where similar expectedFailure is placed and I think we are good this time 🤞

ngimel · 2022-06-28T18:54:40Z

torch/testing/_internal/common_methods_invocations.py

+                    rhs_make_tensor_kwargs=dict(exclude_zero=False),
+                    skips=(
+                        # clamp_max supports two tensor input with bool, but not a bool scalar
+                        DecorateInfo(unittest.expectedFailure, 'TestCommon', 'test_dtypes'),


Some sample inputs failing for dtype shouldn't result in test failure, so what's going on here?

This test was complaining about missing torch.bool and torch.float16 in dtypes.

====================================================================== FAIL: test_dtypes_clamp_max_cpu (__main__.TestCommonCPU) ---------------------------------------------------------------------- Traceback (most recent call last): File "/opt/pytorch/pytorch/torch/testing/_internal/common_device_type.py", line 377, in instantiated_test result = test(self, **param_kwargs) File "/opt/pytorch/pytorch/torch/testing/_internal/common_device_type.py", line 786, in test_wrapper return test(*args, **kwargs) File "/opt/pytorch/pytorch/torch/testing/_internal/common_device_type.py", line 821, in dep_fn return fn(slf, *args, **kwargs) File "/opt/pytorch/pytorch/torch/testing/_internal/common_device_type.py", line 979, in only_fn return fn(self, *args, **kwargs) File "test_ops.py", line 314, in test_dtypes self.fail(msg) AssertionError: The supported dtypes for clamp_max on device type cpu are incorrect! The following dtypes worked in forward but are not listed by the OpInfo: {torch.bool, torch.float16}. The following dtypes worked in backward but are not listed by the OpInfo: {torch.float16}.

The comment here # clamp_min supports two tensor input with bool, but not a bool scalar was referring to the failure on a different test when I add torch.bool in the supported dtype. (I think I also mistakenly set rhs_python_scalar=True then).

jjsjann123 · 2022-06-29T13:59:23Z

@pytorchbot merge

pytorchmergebot · 2022-06-29T14:12:18Z

@pytorchbot successfully started a merge job. Check the current status here

pytorchmergebot · 2022-06-29T14:12:27Z

@jjsjann123 your PR has been successfully merged.

github-actions · 2022-06-29T14:13:26Z

Hey @jjsjann123.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

Summary: Added reference implementation for the two ops; Added opinfo tests for aten clamp_min/clamp_max; Added opinfo reference test. Pull Request resolved: #79821 Approved by: https://github.com/mruberry Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/c28315eab851b9d126457738d73deae0cccfc2bc Reviewed By: b0noI Differential Revision: D37523050 fbshipit-source-id: e0d72fadf88700b97a577d580ecd3cfb1034101c

jjsjann123 added 15 commits June 17, 2022 14:51

clamp_min/max support

5eedd15

clamp_min/max support

98a0fb7

clamp_min/max support

fde6ac0

patching tests

cce1c2c

patching tests

60a739a

patching tests

1ae170b

patching tests

37aadfd

patching tests

02036c3

patching tests

00de23b

patching tests

a3aa2ee

patching tests

cbf5773

patching tests

6a3c01b

patching tests

d83f86d

patching tests

5c30ed7

patching tests

09bd9fb

jjsjann123 requested review from mruberry and ngimel as code owners June 17, 2022 22:46

facebook-github-bot added the cla signed label Jun 17, 2022

jjsjann123 commented Jun 17, 2022

View reviewed changes

pytorchbot added the open source label Jun 17, 2022

ngimel reviewed Jun 17, 2022

View reviewed changes

mruberry reviewed Jun 17, 2022

View reviewed changes

albanD added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jun 21, 2022

jjsjann123 added 4 commits June 22, 2022 02:29

Merge remote-tracking branch 'jiej/master' into HEAD

2e385fb

updating from using minimum/maximum to where for gradient behavior

9ceec45

updating clamp implementation

df52486

fixing clamp

4f4dd5b

jjsjann123 added 4 commits June 23, 2022 04:55

clamp_min/max doesn't accept scalar tensor

e577fd1

clamp_min/max disables type promotion test

bb80a32

clamp_min/max dispatch to lazy failed

7153bde

clamp_min/max disable error tests

105d41e

jjsjann123 added 2 commits June 24, 2022 11:53

Merge commit '3afc802c5a5111' into HEAD

1a25952

code cleaning/refactoring

1476493

jjsjann123 added 2 commits June 24, 2022 12:37

errr, typo

3d34adc

lintrunner

4bbf33c

jjsjann123 commented Jun 24, 2022

View reviewed changes

mruberry reviewed Jun 27, 2022

View reviewed changes

mruberry approved these changes Jun 27, 2022

View reviewed changes

jjsjann123 added 3 commits June 28, 2022 05:26

Merge commit '4331bc436ea' into HEAD

affe449

skip -> expected failure

a1272bc

updating expected failure

1192aab

mruberry reviewed Jun 28, 2022

View reviewed changes

ngimel reviewed Jun 28, 2022

View reviewed changes

jjsjann123 added 2 commits June 28, 2022 14:46

updating supported dtypes

02f0128

typo

a741bcd

pytorchmergebot added the Merged label Jun 29, 2022

pytorchmergebot closed this in c28315e Jun 29, 2022

		yield SampleInput(a, args=(b, c))


		def _clamp_min_numpy(a, min=None):

[primtorch] add reference for clamp_min/clamp_max #79821

[primtorch] add reference for clamp_min/clamp_max #79821

Uh oh!

Conversation

jjsjann123 commented Jun 17, 2022

Uh oh!

facebook-github-bot commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngimel commented Jun 23, 2022

Uh oh!

jjsjann123 commented Jun 23, 2022

Uh oh!

jjsjann123 commented Jun 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjsjann123 commented Jun 29, 2022

Uh oh!

pytorchmergebot commented Jun 29, 2022

Uh oh!

pytorchmergebot commented Jun 29, 2022

Uh oh!

github-actions bot commented Jun 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

facebook-github-bot commented Jun 17, 2022 •

edited

Loading