-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use torch.testing.assert_close
for internal test suite
#58981
Conversation
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit d3ad51c (more details on the Dr. CI page):
🕵️ 14 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages: Windows CI (pytorch-win-vs2019-cpu-py3) / test (default, 2, 2, windows.4xlarge) (1/14)Step: "Run test scripts" (full log | diagnosis details | 🔁 rerun)
|
Blocked by #59067. |
Complex comparison with conjugate bit set (for example see this CI run) will be fixed in #60522. |
@mruberry there are multiple test that fail now because the values are not close within the default tolerances, whereas they passed before. For example see this CI run. Although pytorch/torch/testing/_internal/common_utils.py Lines 1209 to 1215 in 3f3fd57
and afterwards equalizes the pytorch/torch/testing/_asserts.py Lines 51 to 53 in 66452e0
I think the way it is done in >>> actual = torch.tensor(0.99, dtype=torch.bfloat16)
>>> expected = torch.tensor(1.0, dtype=torch.float64)
>>> torch.testing.assert_close(actual, expected, check_dtype=False)
AssertionError: Tensors are not close!
Mismatched elements: 1 / 1 (100.0%)
Greatest absolute difference: 0.01171875 at 0 (up to 1e-07 allowed)
Greatest relative difference: 0.01171875 at 0 (up to 1e-07 allowed)
>>> torch.testing.assert_close(actual, expected.to(actual), check_dtype=False) |
@mruberry There are multiple failures throughout the test suite, because It is hard for me to judge if these are actual bugs or they were just overlooked while writing the test. Thoughts? |
@mruberry There are multiple failures throughout the test suite like this:
As discussed offline and for example in #56544 (comment) this could be fixed by an additional I vote against making |
`torch.isclose` does not do this bool tensors, which results in a test failure since subtraction (`abs(actual - expected)`) is not supported for them (see #58981). Since the `dtype` is already checked at this point, we can safely move the upcasting before `torch.isclose` is invoked. [ghstack-poisoned]
`torch.isclose` does not do this bool tensors, which results in a test failure since subtraction (`abs(actual - expected)`) is not supported for them (see #58981). Since the `dtype` is already checked at this point, we can safely move the upcasting before `torch.isclose` is invoked. ghstack-source-id: 7214ffb92f4851e8f5edc9100ed81bdcd5f9db4d Pull Request resolved: #60536
The few examples I looked at just seem like they're taking advantage of the "shorthand" that assertEqual allowed. I think changing them is fine. One option for adopting assert_close as the guts of assertEqual (instead of compareTensors) would be to have each test class define an attribute, like "use_assert_close", and if that attribute is true they call assert_close internally. That would allow going test class by test class if switching everything at once was too laborious. |
Would your overall proposal be something like |
We now have three out of three datapoints that `check_stride` will be `partial`'ed to `False`: - `torch`: #58981 (comment) - `torchvision`: #56544 (comment) - `kornia`: https://github.com/kornia/kornia/blob/9041c42b410e6a4bbb664c7134a120be80aa2265/test/utils.py#L25 Given that the strides in most cases are in implementation detail, IMO we should change the default to `False`. In cases were matching strides is a requirement for closeness / equality it can always set to `True` manually. [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry [ghstack-poisoned]
Supersedes #58981. cc mruberry [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry [ghstack-poisoned]
Supersedes #58981. cc mruberry [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry [ghstack-poisoned]
Supersedes #58981. cc mruberry [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry [ghstack-poisoned]
Supersedes #58981. cc mruberry [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry [ghstack-poisoned]
Supersedes #58981. cc mruberry [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry [ghstack-poisoned]
Supersedes #58981. cc mruberry [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry [ghstack-poisoned]
Supersedes #58981. cc mruberry [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry [ghstack-poisoned]
Supersedes #58981. cc mruberry [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry Differential Revision: [D33542994](https://our.internmc.facebook.com/intern/diff/D33542994) [ghstack-poisoned]
Supersedes #58981. cc mruberry Differential Revision: [D33542994](https://our.internmc.facebook.com/intern/diff/D33542994) [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry Differential Revision: [D33542994](https://our.internmc.facebook.com/intern/diff/D33542994) [ghstack-poisoned]
Supersedes #58981. cc mruberry Differential Revision: [D33542994](https://our.internmc.facebook.com/intern/diff/D33542994) [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry Differential Revision: [D33542994](https://our.internmc.facebook.com/intern/diff/D33542994) [ghstack-poisoned]
Supersedes #58981. cc mruberry Differential Revision: [D33542994](https://our.internmc.facebook.com/intern/diff/D33542994) [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry Differential Revision: [D33542994](https://our.internmc.facebook.com/intern/diff/D33542994) [ghstack-poisoned]
Supersedes #58981. cc mruberry Differential Revision: [D33542994](https://our.internmc.facebook.com/intern/diff/D33542994) [ghstack-poisoned]
…ase.assertEqual`" Supersedes #58981. cc mruberry Differential Revision: [D33542994](https://our.internmc.facebook.com/intern/diff/D33542994) [ghstack-poisoned]
Supersedes #58981. cc mruberry Differential Revision: [D33542994](https://our.internmc.facebook.com/intern/diff/D33542994) [ghstack-poisoned]
With this we partially use the newly added
torch.testing.assert_close
for the internal numeric comparisons intorch.testing._internal.common_utils.TestCase.assertEqual
that is widely used in our test suite.Some limitations:
torch.testing.assert_close
supports nested containers of numerics, butself.assertEqual
is also used to check containers of arbitrary type. Since it calls itself recursively,torch.testing.assert_allclose
will still be hit for the items.torch.testing.assert_close
#58844 adds support for checking sparse tensors withtorch.testing.assert_allclose
and thus we cannot replace the sparse code just yet.torch.testing.assert_close
#58926 adds support for checking quantized tensors withtorch.testing.assert_allclose
and thus we cannot replace the quantized code just yet.