Skip to content
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

Kokkos::complex<T> behaves slightly differently from std::complex<T> #1011

Closed
bjoo opened this issue Aug 1, 2017 · 5 comments
Closed

Kokkos::complex<T> behaves slightly differently from std::complex<T> #1011

bjoo opened this issue Aug 1, 2017 · 5 comments
Assignees
Labels
Enhancement Improve existing capability; will potentially require voting
Milestone

Comments

@bjoo
Copy link
Contributor

bjoo commented Aug 1, 2017

Just came accross this issue today. In some cases I want to set the real and imaginary parts of a Kokkos::complex separately. This works in the intuitive way e.g.

Kokkos::complex f;
f.real() = 3.0;
f.imag() = 4.0;

However, when I tried to switch over to std::complex this no longer worked. Apparently
now the only way to do this is:

std::complex f;
f.real(3.0);
f.imag(4.0);

When I tried this with Kokkos::complex it didn't work:

/global/homes/b/bjoo/MG/mg_proto_vec/test/kokkos/./kokkos_vectype.h(222): error: no instance >of overloaded function "Kokkos::complex::imag [with RealType=REAL={REAL64=?{double}}]" matches the argument list
argument types are: (double)
object type is: MG::MyComplex
adata[i].imag( adata[i].imag() - sign*bdata[i].real() );
^
/global/homes/b/bjoo/MG/mg_proto_vec/kokkos/core/src/Kokkos_Complex.hpp(221): note: this >candidate was rejected because mismatch in count of arguments
KOKKOS_INLINE_FUNCTION const RealType imag () const volatile {

(here MG::MyComplex is aliased to Kokkos::Complex using using)

Ultimately this may not matter as folks should use Kokkos::complex, but it may trip up users migrating form std::complex . For what its worth, I prefer the Kokkos version as I can do things like

a.real() += b.imag();

@mhoemmen
Copy link
Contributor

mhoemmen commented Aug 2, 2017

Hi @bjoo ! Would you like to submit a pull request (against Kokkos' develop branch) in order to fix this? Also, is the requested feature standard with C++11? Does it work with GCC 4.8.4?

@ibaned ibaned added the Enhancement Improve existing capability; will potentially require voting label Aug 2, 2017
@ibaned ibaned added this to the 2017-September (mid) milestone Aug 2, 2017
@ibaned ibaned self-assigned this Aug 2, 2017
@ibaned
Copy link
Contributor

ibaned commented Aug 2, 2017

Here is a relevant page of C++ documentation with annotations about how things change over standards.
I suspect the way they did it this way (getters and setters) is that there is no guarantee that std::complex internally actually just contains two members of type T, it may be using some other weird encoding. That said, the getters are constexpr since C++11, so it can't be that weird.

@ibaned
Copy link
Contributor

ibaned commented Aug 2, 2017

Just added these functions, tested them, and got them approved into Kokkos develop. Record time!

@bjoo
Copy link
Contributor Author

bjoo commented Aug 2, 2017 via email

@mhoemmen
Copy link
Contributor

mhoemmen commented Aug 2, 2017

@bjoo Kokkos::complex<T> is just a struct { T real_; T imag_; };. It only exists because std::complex doesn't (currently) work on GPUs and doesn't have the required volatile overloads for use as a Kokkos::parallel_{reduce,scan} reduction result.

If you're worried about vectorization on CPUs, you might like to try the built-in C99 types double _Complex or float _Complex. Their syntax is a little bit different, though. I wrote Kokkos::complex because Tpetra had already committed well before my time to use std::complex, so I needed a drop-in replacement.

@ibaned ibaned closed this as completed Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve existing capability; will potentially require voting
Projects
None yet
Development

No branches or pull requests

3 participants