-
Notifications
You must be signed in to change notification settings - Fork 24.9k
Support operations on c10::complex and integer scalars #38418
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
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit d85628a (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 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. This comment has been revised 37 times. |
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
Thanks for improving this! |
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers performance benefits. This brings back #34506 (which was made for std::complex). [ghstack-poisoned]
test_binary_ops_for_int_type_<T, int64_t>(real, img, i); | ||
} | ||
|
||
TEST(TestArithmeticIntScalar, All) { |
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.
@zasdfgbnm I ended up using the TEST
macro to avoid the Windows CI error
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.
That is OK, could you add a comment?
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.
Probably make a separate PR and add some Windows CUDA comments somewhere? I'm not sure what the true reason this is (but I'm aware that Windows CUDA has a lot of stern requirements). Perhaps @ezyang knows more about this? Thanks!
The Windows error is:
C:/Users/circleci/project\c10/test/util/complex_test_common.h(407): error: calling a __host__ function from a __host__ __device__ function is not allowed
detected during instantiation of "void arithmetic::test_binary_ops_for_all_int_types_(T, T, int8_t) [with T=float]"
(414): here
C:/Users/circleci/project\c10/test/util/complex_test_common.h(408): error: calling a __host__ function from a __host__ __device__ function is not allowed
detected during instantiation of "void arithmetic::test_binary_ops_for_all_int_types_(T, T, int8_t) [with T=float]"
(414): here
C:/Users/circleci/project\c10/test/util/complex_test_common.h(409): error: calling a __host__ function from a __host__ __device__ function is not allowed
detected during instantiation of "void arithmetic::test_binary_ops_for_all_int_types_(T, T, int8_t) [with T=float]"
(414): here
C:/Users/circleci/project\c10/test/util/complex_test_common.h(410): error: calling a __host__ function from a __host__ __device__ function is not allowed
detected during instantiation of "void arithmetic::test_binary_ops_for_all_int_types_(T, T, int8_t) [with T=float]"
(414): here
The change that makes the Windows error message go away:
diff --git a/c10/test/util/complex_test_common.h b/c10/test/util/complex_test_common.h
index 4fde8da2367e..fe61ca0e231d 100644
--- a/c10/test/util/complex_test_common.h
+++ b/c10/test/util/complex_test_common.h
@@ -390,27 +390,27 @@ MAYBE_GLOBAL void test_arithmetic() {
}
template<typename T, typename int_t>
-C10_HOST_DEVICE void test_binary_ops_for_int_type_(T real, T img, int_t num) {
+void test_binary_ops_for_int_type_(T real, T img, int_t num) {
c10::complex<T> c(real, img);
static_assert(c + num == c10::complex<T>(real + num, img), "");
static_assert(num + c == c10::complex<T>(num + real, img), "");
@@ -403,7 +403,7 @@ inline void test_binary_ops_for_int_type_(T real, T img, int_t num) {
}
template<typename T>
-C10_HOST_DEVICE void test_binary_ops_for_all_int_types_(T real, T img, int8_t i) {
+void test_binary_ops_for_all_int_types_(T real, T img, int8_t i) {
test_binary_ops_for_int_type_<T, int8_t>(real, img, i);
test_binary_ops_for_int_type_<T, int16_t>(real, img, i);
test_binary_ops_for_int_type_<T, int32_t>(real, img, i);
test_binary_ops_for_int_type_<T, int64_t>(real, img, i);
}
-MAYBE_GLOBAL void test_arithmetic_int_scalar() {
+TEST(TestArithmeticIntScalar, All) {
test_binary_ops_for_all_int_types_<float>(1.0, 0.1, 1);
test_binary_ops_for_all_int_types_<double>(-1.3, -0.2, -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.
I don't know, it looks like you have declared everything as C10_HOST_DEVICE
where it should be. And sure, feel free to do it in a separate PR.
test_binary_ops_for_int_type_<T, int64_t>(real, img, i); | ||
} | ||
|
||
TEST(TestArithmeticIntScalar, All) { |
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.
That is OK, could you add a comment?
Stack from ghstack:
This is useful in reducing verbosity in c10::complex's general usage, and potentially also offers
performance benefits.
This brings back #34506 (which was made for std::complex).
Differential Revision: D21587012