Support cuComplex.h in `cupy.RawKernel` and `cupy.RawModule` by leofang · Pull Request #2551 · cupy/cupy

leofang · 2019-10-18T04:47:10Z

Closes #1866. Closes #2111.

Previously the support of complex numbers in cupy.RawKernel (and thus cupy.RawModule) was poorly documented, see #1866 (comment) and #2111 for example. This PR aims to solve this problem once and for all. This PR supersedes #1866.

The support of the APIs from cuComplex.h is enabled by prepending a set of C macros that replace those APIs by their Thrust counterparts. Thus, users can just copy and paste their cuComplex-based codes to cupy.RawKernel or cupy.RawModule, and make them work without any modification. Note that the complex mathematical functions are supported without any additional macros.

For backward compatibility, the newly added option enable_cuComplex is set to False by default.

attn: @grlee77

leofang · 2019-10-18T04:48:33Z

Note that the commits in #1866 are taken over to here.

leofang · 2019-10-18T05:20:16Z

cupy/core/carray.pxi

+    #define cuCrealf                      real
+    #define cuCimagf                      imag


This is enabled by #2520.

leofang · 2019-10-18T05:20:38Z

cupy/core/carray.pxi

+    #define cuCreal                       real
+    #define cuCimag                       imag


hvy · 2019-10-21T05:03:01Z

Sorry for the drop by comment but thanks for the PR @leofang. @kmaehashi could you have a look at this PR? If you're busy, then @asi1024 said he might be able to take over.

leofang · 2019-10-21T14:03:51Z

@hvy No need to apologize! Thanks for your followup and kind reply.

kmaehashi · 2019-10-24T07:56:58Z

cupy/core/carray.pxi

+
+    # First, we comment out the line that includes cuComplex.h
+    for i, line in enumerate(source_lines):
+        if '#include' in line and 'cuComplex.h' in line:


Well, this approach sounds fragile and may surprise users. Do we really need to support cuComplex.h? How about providing a bridge interface in the separate header file (in that case users need to replace cuComplex.h to sth like cupy/cuComplex_bridge.h)?

Thanks for raising your concern, @kmaehashi.

Do we really need to support cuComplex.h?

Short answer: Yes. Please do.

Long answer: #2111 is just a long list of the cases I have encountered that request this. Please do not surprise our users. Many scientific projects need complex numbers, but some existing CUDA projects either do not want to depend on Thrust, or it's too large to do a clean overhaul to transit to Thrust's complex types. So, when people port code to CuPy, they are often frustrated. I've helped many people resolve this, and I think this PR solves the issue once and for all. (So people would stop bothering their local CuPy expert, i.e., me...)

If you are really, really against such a support, can you please at least merge #1866? It's hanging there for quite a while, and it's really frustrating. In either case, we need to make it clear to the users. No surprises.

How about providing a bridge interface in the separate header file (in that case users need to replace cuComplex.h to sth like cupy/cuComplex_bridge.h)?

I do not think this is less fragile than the current Pythonic approach. First, this requires code change (although it's only one line), which is against the design principle I had in mind when pushing for RawModule. (Users should just import the source code and let CuPy take care of the rest to make it work.)

Second, in the end the macros in this PR go into cupy/cuComplex_bridge.h that you suggested, but the net effect, after the C preprocessing, is identical. So I don't see why we wanna bother users to make a change?

If you are still concerned and not fully convinced, may I propose to mark the new enable_cuComplex flag as experimental and subject to change? Let me reiterate that the changes in this PR are not enabled by default, so normal use cases would just continue to work (as you will see from Jenkins). It would not hurt to consider this PR.

Thanks again, @kmaehashi.

@kmaehashi just check in. Any comment? Thanks.

OK, now I understand your motivation to use cuComplex code directly on CuPy RawModule/RawKernel.

Regarding the option name, how about translate_cucomplex instead of enable_cuComplex? (1) It is difficult for users to imagine that the code is automatically rewritten when enable is set. (2) pep8 recommends lowercase for variable names.

Could you make the option a keyword-only argument so that we can easily consider reorder/renaming later?

Please use more stricter regex that matches the whole line to detect a header file. You need to consider cases like: # include <cuComplex.h> or // #include <cuComplex.h>

Could you extract macro contents as a separate file (cupy/cuComplex_bridge.h)?
Then replace #include <cuComplex.h> with #include <cupy/cuComplex_bridge.h> // translate_cucomplex to minimize the amount of modification? (this also keeps the original line numbers so it's better for cuda-gdb)

Thank you very much for your understanding and detailed comments, @kmaehashi. I'm working on it.

Two quick replies:

Could you make the option a keyword-only argument so that we can easily consider reorder/renaming later?

If I understand you correctly, I don't think for compile_with_cache this is possible unfortunately. cdef and cpdef functions do not support **kwargs in the end of the function signature (will get an Expected ')', found '**' error).

this also keeps the original line numbers so it's better for cuda-gdb

This is a very good suggestion which I would never come up myself! I never used cuda-gdb after all...

If I understand you correctly, I don't think for compile_with_cache this is possible unfortunately. cdef and cpdef functions do not support kwargs in the end of the function signature (will get an Expected ')', found '' error).

Yes, you're correct. I think it's OK for compile_with_cache as it is not public (documented) interface.

leofang · 2019-11-08T17:38:03Z

Done. PTAL.

cupy/core/carray.pxi

cupy/core/raw.pxd

cupy/core/raw.pyx

cupy/core/include/cupy/cuComplex_bridge.h

leofang · 2019-11-11T06:33:22Z

The requested changes are done except for the regex one. Tested locally.

cupy/core/carray.pxi

cupy/core/raw.pyx

kmaehashi

Some more nitpicks

cupy/core/carray.pxi

Co-authored-by: Kenichi Maehashi <webmaster@kenichimaehashi.com>

Co-Authored-By: Kenichi Maehashi <webmaster@kenichimaehashi.com>

leofang · 2019-11-11T08:21:44Z

@kmaehashi Most requested changes are done except for the **kwargs comment.

Also, I took the liberty of removing six-related code in cupy/core/raw.pyx. Since PY2 support is dropped, I hope this is acceptable. If not, I'll bring them back.

Thanks.

cupy/core/raw.pyx

kmaehashi · 2019-11-11T08:31:29Z

pfnCI, test this please.
(Running tests as this PR is almost complete.)

pfn-ci-bot · 2019-11-11T08:31:33Z

Successfully created a job for commit 9c02d56:

Dashboard for commit 9c02d56

chainer-ci · 2019-11-11T09:17:11Z

Jenkins CI test (for commit 9c02d56, target branch master) failed with status FAILURE.

leofang · 2019-11-12T01:09:18Z

Everything seems to work. The only CI failure seems unrelated. The six support is restored, and the keyword-only argument is used. @kmaehashi PTAL, thanks.

kmaehashi

LGTM!

kmaehashi · 2019-11-12T06:26:49Z

pfnCI, test this please.

pfn-ci-bot · 2019-11-12T06:26:53Z

Successfully created a job for commit 0ff24d7:

Dashboard for commit 0ff24d7

chainer-ci · 2019-11-12T09:23:32Z

Jenkins CI test (for commit 0ff24d7, target branch master) failed with status FAILURE.

kmaehashi · 2019-11-12T10:56:59Z

Test failures are not related to this PR.

leofang · 2019-11-12T12:29:25Z

Thanks a lot @kmaehashi for your help and @grlee77 for the update in the docs!

* upstream/master: (28 commits) apply review (need six; use keyword-only args) Apply suggestions from code review use regex apply review apply review from cupy#2551 (comment) Try using a shape of `()` for `tobytes` testing Update tests/cupy_tests/core_tests/test_ndarray_conversion.py Implement `tobytes` for CuPy arrays apply review Type `dumps` return value as `bytes` silence flake8 fix cupy#2610 Update CODE_OF_CONDUCT.md silence flake8 ensure axis is None before device_reduce temporarily removed WIP for device segmented reduce device reduce for argmin and argmax works this compiles but is not yet tested propagate metadata to RawKernels ensure operation order is preserved ...

leofang and others added 7 commits October 17, 2019 23:52

support cuComplex.h in RawKernel and RawModule

5dd3502

make the search scope better defined

394008a

add tests

7c69491

DOC: add RawKernel example using complex-valued arrays

ad58381

clarify that cuComplex.h should not be used

80cc83d

fix backticks

d735d74

document enable_cuComplex

91d6d44

leofang mentioned this pull request Oct 18, 2019

Recent refurbishment of RawKernel #2450

Closed

9 tasks

leofang commented Oct 18, 2019

View reviewed changes

cupy/core/carray.pxi Outdated

Comment on lines +156 to +157

#define cuCreal real

#define cuCimag imag

Copy link
Copy Markdown

Member Author

leofang Oct 18, 2019

Choose a reason for hiding this comment

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

Ditto.

added more tests; code re-ordered

cadbae0

leofang force-pushed the cuComplex branch from c4486be to cadbae0 Compare October 18, 2019 14:51

ensure operation order is preserved

04b2e15

hvy assigned kmaehashi Oct 21, 2019

propagate metadata to RawKernels

fe5f86d

kmaehashi reviewed Oct 24, 2019

View reviewed changes

apply review from cupy#2551 (comment)

57790f9

kmaehashi requested changes Nov 11, 2019

View reviewed changes

apply review

42a7500

kmaehashi reviewed Nov 11, 2019

View reviewed changes

cupy/core/carray.pxi Outdated Show resolved Hide resolved

cupy/core/raw.pyx Outdated Show resolved Hide resolved

kmaehashi reviewed Nov 11, 2019

View reviewed changes

cupy/core/carray.pxi Outdated Show resolved Hide resolved

cupy/core/carray.pxi Outdated Show resolved Hide resolved

leofang and others added 2 commits November 11, 2019 03:05

use regex

22470ff

Co-authored-by: Kenichi Maehashi <webmaster@kenichimaehashi.com>

Apply suggestions from code review

9c02d56

Co-Authored-By: Kenichi Maehashi <webmaster@kenichimaehashi.com>

kmaehashi requested changes Nov 11, 2019

View reviewed changes

cupy/core/raw.pyx Outdated Show resolved Hide resolved

cupy/core/raw.pyx Show resolved Hide resolved

kmaehashi added cat:enhancement Improvements to existing features takeover Pull-requests taken over from other contributor labels Nov 11, 2019

apply review (need six; use keyword-only args)

0ff24d7

kmaehashi approved these changes Nov 12, 2019

View reviewed changes

kmaehashi merged commit af7e41e into cupy:master Nov 12, 2019

kmaehashi added this to the v7.0.0 milestone Nov 12, 2019

leofang deleted the cuComplex branch November 12, 2019 12:10

leofang mentioned this pull request Nov 12, 2019

DOC: add RawKernel example using complex-valued arrays #1866

Closed

Uh oh!

Conversation

leofang commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leofang commented Oct 18, 2019

Uh oh!

leofang Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

leofang Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

hvy commented Oct 21, 2019

Uh oh!

leofang commented Oct 21, 2019

Uh oh!

kmaehashi Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

leofang Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leofang Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

kmaehashi Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

leofang Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

kmaehashi Nov 11, 2019

Choose a reason for hiding this comment

Uh oh!

leofang commented Nov 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leofang commented Nov 11, 2019

Uh oh!

Uh oh!

Uh oh!

kmaehashi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leofang commented Nov 11, 2019

Uh oh!

Uh oh!

Uh oh!

kmaehashi commented Nov 11, 2019

Uh oh!

pfn-ci-bot commented Nov 11, 2019

Uh oh!

chainer-ci commented Nov 11, 2019

Uh oh!

leofang commented Nov 12, 2019

Uh oh!

kmaehashi left a comment

Choose a reason for hiding this comment

Uh oh!

kmaehashi commented Nov 12, 2019

Uh oh!

pfn-ci-bot commented Nov 12, 2019

Uh oh!

chainer-ci commented Nov 12, 2019

Uh oh!

kmaehashi commented Nov 12, 2019

Uh oh!

leofang commented Nov 12, 2019

Uh oh!

Reviewers

leofang commented Oct 18, 2019 •

edited

Loading

leofang Oct 24, 2019 •

edited

Loading