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

[WIP] Add cupy.complex32 #4454

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

[WIP] Add cupy.complex32 #4454

wants to merge 9 commits into from

Conversation

leofang
Copy link
Member

@leofang leofang commented Dec 17, 2020

First step toward #3370. Related to #4407. DECISION NEEDED BEFORE ANY CODE REVIEW

This is a working prototype to demonstrate that we can define a new Python type (cupy.complex32) and the associated NumPy dtype (np.dtype(cupy.complex32)) so that this type object can be used to hint that the input array consists of half-precision complex numbers, to be used internally wherever its siblings cupy.complex64 and cupy.complex128 are applicable. The subsequent GPU operations is out of scope in this PR and will be addressed when this is merged.

Currently it works for:

  • Type identification
  • Correct inheritance (ex: cupy.complex32 inherits from numpy.complexfloating, same as other complex types)
  • Correct itemsize (4 bytes) for the purpose of array allocation and indexing
  • Correct type kind ('c' for complex)
  • Meaningful type character (uppercase 'E' for 2 float16 as complex, as oppose to lowercase 'e' for 1 float16 as real)
  • 4-byte alignment
  • Very limited functionalities (see below)

Critical decision to make before moving on: I incline to consider it acceptable that it has very limited functionality when used as a dtype. In particular, I do not wish to support all kinds of CPU ufuncs or array funcs (as defined in this section) when manipulated as a NumPy complex32 array, because it would then be more suitable for a PR to NumPy, not to CuPy, and as far as I know NumPy folks do not want to add more new built-in types (numpy/numpy#14753), nor do I have the bandwidth to do so. I think having this type object defined is already enough for GPU needs. If it's acceptable then I will work on polishing this.

@leofang
Copy link
Member Author

leofang commented Jan 4, 2021

The discussion is ongoing in numpy/numpy#14753!

@emcastillo
Copy link
Member

emcastillo commented Jan 12, 2021

We have been talking offline, maybe it's better to postpone this until NumPy implements complex32.
We don't want to do it ourselves first as it may lead to some incompatibilities 😁

@leofang
Copy link
Member Author

leofang commented Jan 18, 2021

So let me quickly summarize the discussion in numpy/numpy#14753 so far. It's doable and the Numpy core devs generally support it. We agree that this dtype will be called numpy.complex32 with the single character representation 'E', as chosen in #4407 and this PR. NumPy's new dtype system will be finalized hopefully in a few months, and the questions left are how to add complex32 to their type hierarchy, and whether or not to make it as "odd" as float16 (see the discussion therein to know what they meant by "odd"; as far as CuPy is concerned, it's just another dtype).

@leofang
Copy link
Member Author

leofang commented Jan 18, 2021

I'd like to note, though, that if CuPy would just alias numpy.complex32, it means this feature won't be available in CuPy for at least 1 year or more, until the new NumPy version (the earliest one in which complex32 is added) becomes our lowest supported NumPy...

@emcastillo
Copy link
Member

I'd like to note, though, that if CuPy would just alias numpy.complex32, it means this feature won't be available in CuPy for at least 1 year or more, until the new NumPy version (the earliest one in which complex32 is added) becomes our lowest supported NumPy...

We discussed this, we can implement it once it is released in NumPy master branch, as we always support the latest released NumPy version during development. However, we will need to disable it for older NumPy versions.

@leofang
Copy link
Member Author

leofang commented Feb 25, 2021

Let me close this to clean up my PR queue 😁 I'll leave the branch alive for reference. This need is already tracked in #3370.

@leofang leofang closed this Feb 25, 2021
@asi1024 asi1024 added this to the Closed PRs milestone Feb 25, 2021
@leofang
Copy link
Member Author

leofang commented May 11, 2023

Hmmm I can't reopen because the target branch (master) is gone. @kmaehashi or @emcastillo, can you try changing the target branch to main? It seems don't have access to do that. If it doesn't work, I'll open a new PR. Thanks!

@kmaehashi kmaehashi reopened this May 11, 2023
@kmaehashi kmaehashi changed the base branch from master to main May 11, 2023 09:31
@kmaehashi kmaehashi removed this from the Closed PRs milestone May 17, 2023
@leofang
Copy link
Member Author

leofang commented Jul 11, 2023

Sorry for delay. @seberg has kindly taken a look at this PR, and agreed I was on the right track. What I'll do is to clean up/polish the code, hook it up with the build system, and add tests. I'll ping Sebastian and the team once it's ready.

@kmaehashi kmaehashi added this to the v13.0.0rc1 milestone Aug 10, 2023
@kmaehashi kmaehashi removed this from the v13.0.0rc1 milestone Nov 27, 2023
@kmaehashi
Copy link
Member

Milestone removed.

@leofang
Copy link
Member Author

leofang commented Dec 1, 2023

Yeah unfortunately I can't make it for v13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:enhancement Improvements to existing features st:needs-discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants