Skip to content

Update thrust::complex headers with a bug fix#2741

Merged
emcastillo merged 3 commits intocupy:masterfrom
leofang:update_complex_headers
Dec 19, 2019
Merged

Update thrust::complex headers with a bug fix#2741
emcastillo merged 3 commits intocupy:masterfrom
leofang:update_complex_headers

Conversation

@leofang
Copy link
Copy Markdown
Member

@leofang leofang commented Dec 2, 2019

Closes #2629. Changes (see the discussion starting #2629 (comment)):

  1. Add a __host__ qualifier to all functions
  2. Sync a few function implementations with the upstream counterparts
  3. Adjust some style and format
  4. Fix a bug: Update thrust::complex headers with a bug fix #2741 (comment)

@emcastillo
Copy link
Copy Markdown
Member

Regarding the __host__ qualifiers.
I understand the value of doing it for diffing files with thrust master. However, since this code is going to be device-only, I am a bit reluctant to it.

@leofang
Copy link
Copy Markdown
Member Author

leofang commented Dec 3, 2019

You may be surprised, but it's needed in supporting external libraries like CUB. (The compilation just wouldn't pass without __host__ for some reason.) We've managed to make the change only when needed, but it's annoying and would be good to be kept up-to-date.

Comment on lines -153 to -161
template <typename ValueType>
__host__ __device__ inline bool operator>(const complex<ValueType>& lhs,
const complex<ValueType>& rhs) {
if (lhs == rhs)
{
template <typename T>
__host__ __device__ inline bool operator>(const complex<T>& lhs,
const complex<T>& rhs) {
if (lhs == rhs) {
return false;
} else if (lhs.real() > rhs.real()) {
return true;
} else if (lhs.real() == rhs.real()) {
return lhs.imag() > rhs.imag();
} else {
return false;
} else
{
return !(lhs < rhs);
Copy link
Copy Markdown
Member Author

@leofang leofang Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change in 655c8e3 fixed a bug:

>>> import cupy as cp
>>> a = cp.asarray([cp.complex(cp.nan, 0.9), cp.complex(0.7, cp.nan)])
>>> a[0] < a[1]  # correct
array(False)
>>> a[0] > a[1]  # wrong, should be False to match NumPy
array(True)

@emcastillo
Copy link
Copy Markdown
Member

You may be surprised, but it's needed in supporting external libraries like CUB

😂awesome. No problem with keeping these then.

@leofang leofang changed the title [WIP] Update thrust::complex headers Update thrust::complex headers with a bug fix Dec 16, 2019
@leofang leofang marked this pull request as ready for review December 16, 2019 15:41
@leofang
Copy link
Copy Markdown
Member Author

leofang commented Dec 16, 2019

@emcastillo PTAL.

Copy link
Copy Markdown
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@emcastillo emcastillo added the cat:code-fix Code refactoring that do not change behavior label Dec 18, 2019
@emcastillo emcastillo added this to the v8.0.0a1 milestone Dec 18, 2019
@emcastillo emcastillo added to-be-backported Pull-requests to be backported to stable branch cat:bug Bugs and removed cat:code-fix Code refactoring that do not change behavior labels Dec 18, 2019
@emcastillo
Copy link
Copy Markdown
Member

Jenkins, test this please

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit 655c8e3:

@chainer-ci
Copy link
Copy Markdown
Member

Jenkins CI test (for commit 655c8e3, target branch master) succeeded!

@emcastillo emcastillo merged commit 1e83118 into cupy:master Dec 19, 2019
emcastillo pushed a commit to emcastillo/cupy that referenced this pull request Dec 19, 2019
Update thrust::complex headers with a bug fix
@leofang leofang deleted the update_complex_headers branch December 19, 2019 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cat:bug Bugs to-be-backported Pull-requests to be backported to stable branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sync the headers in cupy/core/include/cupy/complex/ with upstream?

4 participants