Skip to content

Add complex number support to astype #445

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 20, 2022
Merged

Add complex number support to astype #445

merged 4 commits into from
Nov 20, 2022

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented May 30, 2022 •
edited
Loading

This PR:

  • adds complex number support to astype. As astype generally supports copying an array to any specified data type irrespective of type promotion rules, no exception to general behavior is made for complex number arrays.

    Accordingly, following C, NumPy, and others, when casting a complex floating-point array to a real-valued data type, the imaginary component must be discarded and only the real component cast to the desired data type.

Notes

  • The alternative to discarding the imaginary component is to require users call real(x) before calling astype. However, as astype is more generally used to force a type conversion, prohibiting casting complex-valued arrays to real-valued arrays did not seem ideal, nor consistent with, say, casting a floating-point array to an integral array (i.e., we allow casting a floating-point array to an integral array and don't require astype(floor(x))). Hence, this PR chooses to follow existing precedent and require that the imaginary component be discarded when performing the conversion.
In [1]: z = np.asarray(1.5 + 2.6j)

In [2]: z.astype(dtype="float64")
<ipython-input-9-bea51b1612d9>:1: ComplexWarning: Casting complex values to real discards the imaginary part
  z.astype(dtype="float64")
Out[2]: array(1.5)

In [3]: z.astype(dtype="bool")
Out[3]: array(True)

In [4]: np.asarray(True).astype(dtype="complex128")
Out[4]: array(1.+0.j)

In [5]: np.asarray(False).astype(dtype="complex128")
Out[5]: array(0.+0.j)

In [6]: z.astype(dtype="int32")
<ipython-input-13-5f154e24a33c>:1: ComplexWarning: Casting complex values to real discards the imaginary part
  z.astype(dtype="int32")
Out[6]: array(1, dtype=int32)

In [7]: np.asarray(0+0j).astype(dtype="bool")
Out[7]: array(False)

Updates

  • Based on discussions in the consortium workgroup and the 22 September 2022 workgroup meeting, consensus emerged to disallow casting from a complex floating-point dtype to a real-valued dtype. Instead, array API consumers should be explicit in specifying which component should be cast to a specified real-valued dtype.

@kgryte kgryte added API change Changes to existing functions or objects in the API. topic: Complex Data Types Complex number data types. labels May 30, 2022
@kgryte kgryte added this to the v2022 milestone May 30, 2022
@kgryte kgryte mentioned this pull request May 30, 2022
Closed
leofang
leofang reviewed Jun 6, 2022
View reviewed changes
Copy link
Contributor

@leofang leofang left a comment

Choose a reason for hiding this comment

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

As long as we don't raise the annoying warning when casting complex to real, I am fine with merging this PR.

@kgryte
Copy link
Contributor Author

kgryte commented Jun 6, 2022

@leofang Agreed. I don't think there is any appetite for requiring implementations to display a warning (ala NumPy).

@jakirkham
Copy link
Member

jakirkham commented Jun 6, 2022 •
edited
Loading

Was about to say this should raise a warning 😂

That said, I kind of wonder if this is a good idea as opposed to simply forcing users to do .real.astype(...) instead, which seems clearer. Having .astype(...) shortcut this case seems prone to subtle bugs in user code

What about saying .astype(...) behavior is undefined for types that are not complex. IOW this could still be used for upcasting from complex64 to complex128 and implementers might use the behavior defined here, but it shouldn't be expected

@jakirkham
Copy link
Member

Based on discussion in today's meeting it sounds like a warning/error is inconsistent with the implied casting of "unsafe" that the spec currently uses. That said, there may be value in providing users a way to influence this behavior (IOW raise in some of these cases) to avoid subtle bugs. Have raised issue ( #463 ) about adding this via a casting flag.

@jakirkham jakirkham mentioned this pull request Jul 7, 2022
Closed
@leofang
Copy link
Contributor

leofang commented Sep 6, 2022

Quoting myself from #463 (comment):

Based on #427 (real) and #446 (conj) I think we are leaning toward a more restrictive API design as opposed to permissive. I seem to remember the permissive choice was favored during one of the meetings, but I can't recall it was for which library (and I also forgot which side I picked, I'd say restrictive but I am not certain anymore after a long week...😅) That's probably why I let go in #445 (astype). But, for the sake of consistency it'd be nicer if astype does not allow complex -> real?

@kgryte kgryte requested a review from leofang November 3, 2022 06:51
@kgryte
Copy link
Contributor Author

kgryte commented Nov 3, 2022

This PR has now been updated to reflect the current workgroup consensus to disallow casting from a complex floating-point dtype to a real-valued dtype.

@kgryte
Copy link
Contributor Author

kgryte commented Nov 3, 2022 •
edited
Loading

As discussed in the 3 November 2022 meeting, no objections were raised. This PR should be ready for merge.

@leofang would you mind taking one last look and then approving?

rgommers
rgommers approved these changes Nov 16, 2022
View reviewed changes
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM, seems ready to go in. @leofang I'll give it a few days in case you want to have another look

leofang
leofang approved these changes Nov 20, 2022
View reviewed changes
Copy link
Contributor

@leofang leofang left a comment

Choose a reason for hiding this comment

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

Sorry I dropped the ball, LGTM!

@kgryte
Copy link
Contributor Author

kgryte commented Nov 20, 2022

Thanks, @leofang! In it goes!

@kgryte kgryte merged commit 1b4c218 into main Nov 20, 2022
@kgryte kgryte deleted the cmplx-astype branch November 20, 2022 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgommers rgommers rgommers approved these changes

@leofang leofang leofang approved these changes

Assignees
No one assigned
Labels
API change Changes to existing functions or objects in the API. topic: Complex Data Types Complex number data types.
Projects
None yet
Milestone
v2022
Development

Successfully merging this pull request may close these issues.
4 participants
@kgryte @jakirkham @leofang @rgommers