array-api icon indicating copy to clipboard operation
array-api copied to clipboard

Add complex number support to `astype`

Open kgryte opened this issue 2 years ago • 4 comments

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)

kgryte avatar May 30 '22 08:05 kgryte

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

kgryte avatar Jun 06 '22 20:06 kgryte

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 avatar Jun 06 '22 23:06 jakirkham

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 ( https://github.com/data-apis/array-api/issues/463 ) about adding this via a casting flag.

jakirkham avatar Jul 07 '22 17:07 jakirkham

Quoting myself from https://github.com/data-apis/array-api/issues/463#issuecomment-1178393667:

Based on https://github.com/data-apis/array-api/pull/427 (real) and https://github.com/data-apis/array-api/pull/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 https://github.com/data-apis/array-api/pull/445 (astype). But, for the sake of consistency it'd be nicer if astype does not allow complex -> real?

leofang avatar Sep 06 '22 19:09 leofang

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 avatar Nov 03 '22 06:11 kgryte

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?

kgryte avatar Nov 03 '22 17:11 kgryte

Thanks, @leofang! In it goes!

kgryte avatar Nov 20 '22 03:11 kgryte