-
Notifications
You must be signed in to change notification settings - Fork 22k
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
Clarify, make consistent, and test the behavior of logspace when dtype is integral #47647
Conversation
Hi @xuhdev! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but we do not have a signature on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
💊 CI failures summary and remediationsAs of commit 878a465 (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages: pytorch_linux_xenial_py3_clang5_asan_test1 (1/1)Step: "Run tests" (full log | diagnosis details | 🔁 rerun)
|
8343c4d
to
cf01217
Compare
326b298
to
375a47b
Compare
@@ -133,10 +133,10 @@ Tensor& logspace_cuda_out(Tensor& result, Scalar start, Scalar end, c10::optiona | |||
r.fill_(std::pow(base, start.to<double>())); | |||
} else if (isIntegralType(r.scalar_type(), 0)) { | |||
AT_DISPATCH_INTEGRAL_TYPES(r.scalar_type(), "logspace_cuda", [&]() { | |||
float scalar_base = static_cast<float>(base); // Use float to avoid promotion to double | |||
double scalar_base = static_cast<double>(base); |
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.
could you clarify your change here please?
based on the test_tensor_creation_ops.py
change, you are comparing within the device type instead of crossing cpu/gpu. maybe adding a test doing the cpu/gpu cross comparison can help us review this change.
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.
I have added the test. This is to make the implementation consistent with CPU, because the CPU implementation uses double
instead of float
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.
does your added CPU/GPU comparison test break when you use the original float
here?
i guess my point was, cuda impl used float for a reason, and according to the comment on the CPU side, it was "autopromoted". e.g. we might not need that extra precision
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.
@walterddr Yes, it broke when I kept them as float. Actually it broke the tests on CUDA alone. Likely I guess this is about log/exp, and low precision can easily cause trouble.
i guess my point was, cuda impl used float for a reason, and according to the comment on the CPU side, it was "autopromoted". e.g. we might not need that extra precision
My speculation is that the CUDA implementation used float for perhaps performance reasons? cc @colesbury
Also, I'm having a hard time understanding the current asan test failure, which doesn't show what is broken (it still failed for the same reason when I did not add anything to the CUDA implementation).
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.
looks good. I got one more comment and it should be good to go
test/test_tensor_creation_ops.py
Outdated
@@ -2782,6 +2782,20 @@ def test_logspace(self, device, dtype): | |||
y = torch.logspace(0, 3, 4, base=2, device=device, dtype=dtype, out=x.narrow(1, 1, 2)) | |||
self.assertEqual(x, torch.tensor(((0, 1, 2), (0, 4, 8)), device=device, dtype=dtype), atol=0, rtol=0) | |||
|
|||
# Integer test | |||
if torch.testing.is_integral(dtype): |
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.
you can just create a separate test
@skipCPUIf(True, "compares with CPU")
@dtypesIfCUDA(torch.uint8, torch.int8, torch.int16, torch.int32, torch.int64)
def test_logspace_integral(self, device, dtype):
...
see how other special typed logspace test were doing before this one.
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.
I added this. I chose to not skip all CPU tests because there are some parts not concerning CPUs.
torch.logspace doesn't seem to have explained how integers are handled. Add some clarification and some test when dtype is integral.
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.
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
thanks for the change. lgtm now. I'll add @colesbury to see if there's any concern on the float->double type change from the cuda side.
@walterddr merged this pull request in 0ae0fac. |
This pull request has been reverted by 3df5f9c. |
@walterddr Is there a reason for the reversion? |
the change actually broke ASAN instead of being flaky. |
also I should've checked this more carefully before, but seems like torch.logspace doesn't produce the same expected results as numpy.logspace. --> its a matter of whether we should convert the start/stop into integer before or after the fact. I will look into it and iterate on top of this PR |
…e is integral torch.logspace doesn't seem to have explained how integers are handled. Add some clarification and some test when dtype is integral. The CUDA implementation is also updated to be consistent with CPU implementation. Following up pytorch#47647
@walterddr Thanks, please keep me posted. |
@walterddr Do we have any plan to further resolve this issue? |
torch.logspace doesn't seem to have explained how integers are handled.
Add some clarification and some test when dtype is integral.
The CUDA implementation is also updated to be consistent with CPU implementation.