-
Notifications
You must be signed in to change notification settings - Fork 136
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
[FEA]: Expand thrust::complex
to be usable at compile time
#485
Comments
Hi, this looks like a good first issue. I would like to help by contributing to this issue. |
@srinivasyadav18 that is awesome 🎉 If you have any questions feel free to poke here or on our discord server. I am happy to give you a rundown of the required changes |
@miscco Thanks for the quick response. If my understanding is correct, we I understand that testing here : Any further insights would highly appreciated. Thanks! |
You are absolutely right, we cannot use plain I have some ToDo items to unify our support macros, but that is a different topic
Yes those tests need to be changed. We are both lucky and unlucky here. Lets have a look at one of the tests: template <typename T>
struct TestComplexConstructionAndAssignment
{
void operator()()
{
thrust::host_vector<T> data = unittest::random_samples<T>(2);
const T real = data[0];
const T imag = data[1];
{
const thrust::complex<T> construct_from_real_and_imag(real, imag);
ASSERT_EQUAL(real, construct_from_real_and_imag.real());
ASSERT_EQUAL(imag, construct_from_real_and_imag.imag());
}
....
}
}; The basic stategy is to just make the test constexpr and let it return a boolean: template <typename T>
struct TestComplexConstructionAndAssignment
{
_LIBCUDACXX_CONSTEXPR_AFTER_CXX11 bool operator()()
{
thrust::host_vector<T> data = unittest::random_samples<T>(2);
const T real = data[0];
const T imag = data[1];
{
const thrust::complex<T> construct_from_real_and_imag(real, imag);
ASSERT_EQUAL(real, construct_from_real_and_imag.real());
ASSERT_EQUAL(imag, construct_from_real_and_imag.imag());
}
....
return true;
}
}; That way we can test both runtime and compile time via: TestComplexConstructionAndAssignment test{};
test();
#if THRUST_CPP_DIALECT > 11
static_assert(test(), "");
#endif // THRUST_CPP_DIALECT > 11 The main problem we will face is that currently our helper class |
@miscco Thanks! I will make the changes and test it out. |
@miscco I have constexpr'd all the constructors and operators for thrust::complex. template <typename T>
struct TestConstexprComplexConstructionAndAssignment
{
void operator()(void)
{
{
// fixed values, as random number generation is not compile time.
constexpr T real = static_cast<T>(42);
constexpr T imag = static_cast<T>(120);
constexpr thrust::complex<T> construct_from_real_and_imag(real, imag);
// compile time check
static_assert(real == construct_from_real_and_imag.real());
static_assert(imag == construct_from_real_and_imag.imag());
}
}
};
SimpleUnitTest<TestConstexprComplexConstructionAndAssignment, FloatingPointTypes>
TestConstexprComplexConstructionAndAssignmentInstance; This is may not be permanent fix, but this is just an idea that I got.
|
We can go that route, but it would essentially duplicate all the test code which is suboptimal. @senior-zero do we have a plan on when to rewrite the thrust tests to catch2? |
We occasionally discuss this possibility, but even if we decide to, it won't happen soon. |
* Add gcc-6 image * Avoid build breaks due to gcc-6 * Disable failing libcxx tests for gcc-6 * Make tests pass with gcc-6 and c++14 * Make tests pass with gcc-6 and c++17
Is this a duplicate?
Area
Thrust
Is your feature request related to a problem? Please describe.
With the rework of
thrust::complex
to be based uponcuda::std::complex
we can leverage the later onesconstexpr
supportDescribe the solution you'd like
We should expand existing operators / methods with the appropriate constexpr qualifier.
This would also require us to rework the way we internally test
thrust::complex
Usually if there was a
void test_something()
function previously one would split it up into aconstexpr bool test_something_impl()
and then useDescribe alternatives you've considered
No response
Additional context
No response
The text was updated successfully, but these errors were encountered: