-
Notifications
You must be signed in to change notification settings - Fork 25.2k
fix a corner case of torch.arange #80758
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
Conversation
[ghstack-poisoned]
🔗 Helpful links
✅ No Failures (0 Pending)As of commit 22ef735 (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. |
trying to fix #80589, see the two corner cases in the issue. added the two cases in unit tests and add device to the tests. [ghstack-poisoned]
It would have been nice for the device parametrization to be separately stacked (no need to do now, I'll review as is) |
torch.arange(1, 0, -1, out=res2) | ||
self.assertEqual(res1, res2, atol=0, rtol=0) | ||
torch.arange(1, 2, 1, out=res2) | ||
self.assertEqual(res1, res2, atol=0, rtol=0) | ||
|
||
# FloatTensor | ||
res1 = torch.arange(0.6, 0.89, 0.1, out=torch.FloatTensor()) | ||
out = torch.tensor([], dtype=torch.float, device=device) | ||
res1 = torch.arange(0.6, 0.89, 0.1, out=out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good riddance, thx
AT_DISPATCH_ALL_TYPES_AND2(kHalf, kBFloat16, result.scalar_type(), "arange_cpu", [&]() { | ||
using accscalar_t = at::acc_type<scalar_t, false>; | ||
auto xstart = start.to<accscalar_t>(); | ||
auto xend = end.to<accscalar_t>(); | ||
auto xstep = step.to<accscalar_t>(); | ||
|
||
TORCH_CHECK(xstep > 0 || xstep < 0, "step must be nonzero"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol are there some floating point shenanigans why xstep != 0
is invalid here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preexisting condition
r = torch.arange(0, 6, 3, device=device) | ||
self.assertEqual(r.min(), 0) | ||
self.assertEqual(r.max(), 3) | ||
self.assertEqual(r.numel(), 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the new test
r = torch.arange(0, -5, -2, device=device) | ||
self.assertEqual(r.min(), -4) | ||
self.assertEqual(r.max(), 0) | ||
self.assertEqual(r.numel(), 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this too, the test here is so bad woof
onnx failure looks real |
thanks for reviewing. Yes, it turns out in onnx we always cast the args (start, end, step) to desired dtype before computing size.
This PR will change the last one to
but for onnx,
the failed unittest compare torch and onnx with int64, so it works on master but fails with this PR. We may need to change the implementation in torch.onnx to make it consistent. |
trying to fix #80589, see the two corner cases in the issue. added the two cases in unit tests and add device to the tests. [ghstack-poisoned]
@pytorchbot merge -g |
@pytorchbot successfully started a merge job. Check the current status here |
Hey @yuguo68. |
Summary: trying to fix #80589, see the two corner cases in the issue. added the two cases in unit tests and add device to the tests. Pull Request resolved: #80758 Approved by: https://github.com/ezyang Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/5e9136c24cf52d9da2e55ecb2049233c765f913b Reviewed By: mehtanirav Differential Revision: D37687413 Pulled By: yuguo68 fbshipit-source-id: befd1e384ff5d13272314bccac1e830b3c8db8de
Stack from ghstack (oldest at bottom):
trying to fix #80589, see the two corner cases in the issue.
added the two cases in unit tests and add device to the tests.
Edit: the landed version only fixes the first issue in #80589