-
Notifications
You must be signed in to change notification settings - Fork 50
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
RFC: revisit requirement that inputs to real
, imag
, and conj
be complex-valued
#824
Comments
Has this come up in real-world code? I can definitely see wanting to write functions that work on both real and complex dtypes. But I also am curious how often a complex implementation can be directly reused for a real implementation. |
Yes. I have a complex number that is the logarithm of a negative real number. I take the real component to get the magnitude. |
Wouldn't |
It would if |
There must be a misunderstanding here. |
This is a better example I think. |
It seems so. But all that's important is that in my application, there are real and complex dtypes going through the same code, and
Sure, it's simpler. I mentioned the original example because it was the most recent time I've wanted |
It definitely make sense to me for Your example if isdtype(x.dtype, 'real floating'):
return log(abs(x))
else:
return real(log(x)) but maybe there are better examples for |
For re = xp.real(xp.astype(x, dtype='complex128', copy=False)) where It would be useful to see an explicit use case beyond a toy example of where sharing the same code path for real and complex is necessary and performance critical. |
I haven't tried to explain that example in complete detail. If everyone is interested, we can review the code, but I thought the basic outline would suffice.
Yeah, I think that's the only benefit I'm suggesting. I don't have any examples where the extra decision (real vs complex) is performance critical. It is only a line of ternary and a few microseconds. It just seemed that the motivation for disallowing If the decision here is to leave But then I'm curious - why not raise an error when one tries to |
I think the conjugating dot product is a legitimate enough use-case. You can have an expression that's exactly the same for complex and real inputs except for the conjugation, which is actually a no-op on real numbers. So you'd have to have something like
Or do some trick like Also note that casting to complex is the wrong solution here. That does have a real performance impact. Not only do complex arrays take up more memory, but a complex algorithm on an array with 0 imaginary part will be much slower than a real algorithm on an equivalent float array. For example: In [1]: import numpy as np
In [2]: a = np.linspace(0, 10, 1000)
In [3]: b = a + 0j
In [4]: %timeit np.sqrt(a)
450 ns ± 2.88 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)
In [5]: %timeit np.sqrt(b)
3.88 µs ± 8.78 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each) This is only the correct thing to do in cases where you don't get the same expression for real inputs, like the |
Thanks @asmeurer. I think another thing worth mentioning is that a lot of array API standard code is translation from existing code. For instance, I brought this up when translating a file with NumPy code that has 11 uses of
For simplicity, I wrapped But all the little changes can add up to a lot of developer time, so I thought revisiting this might save others some effort in the future. It could indeed be the other way - that by being stricter than the existing array libraries, this prevents bugs - but if we don't have data on that, and we have some evidence of legitimate use cases, it might be OK to follow the precedent set by the libraries. |
Another thing to consider is that astype() is explicitly disallowed from casting complex inputs to real inputs (see note 2 at https://data-apis.org/array-api/latest/API_specification/generated/array_api.astype.html#astype). So There is also a question of imag(). imag on a real array would just be an array of zeros, which would make it not completely cheap (I suppose it could be done as a broadcasted scalar, but that would lead to unexpected behavior under mutation). On the other hand, I think it makes more sense to keep imag consistent with real and conj. By the way, just so we're clear, are you also suggesting that these functions should operate on integer arrays, or only on real floating-point arrays? |
I have also encountered the usability issue with For
This is the problem indeed. It's bad API for a function like In the (relatively rare I think) cases where this is the desired semantics: if isdtype(x.dtype, 'complex floating'):
imag_component = x.imag
else:
imag_component = xp.zeros_like(x) it seems better to let the user write that out explicitly. For completeness, performance: >>> x = np.random.default_rng().standard_normal(10_000_000)
>>> x.dtype
dtype('float64')
>>> xc = x.astype(np.complex128)
>>> %timeit xc.imag
50.3 ns ± 0.154 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
>>> %timeit x.imag
4.43 ms ± 74 µs per loop (mean ± std. dev. of 7 runs, 100 loops each) |
Would it have to, or could it be implemented as a view of a single zero element? It should not be writeable anyway, since the original array would still be of real dtypes, so the imaginary part could not be anything other than zero. |
In principle it could be aliased memory from broadcasting a size-1 array indeed. However, (a) NumPy isn't going to make that change I think, and (b) that still has mismatching semantics where
Currently the returned array is writeable, so there'd be a backwards compat concern for NumPy et al. too. |
Well, in CuPy and Dask array, it looks like. Not NumPy, Torch (doesn't allow import numpy as xp # 1.26 or 2.0
x = xp.asarray([1., 2., 3.])
y = xp.imag(x)
y[1] = 10
# ValueError: assignment destination is read-only
The standard doesn't specify whether libraries should return a view or copy - does it ever specify writability? If not, NumPy wouldn't have to change to be compatible with the standard. It just might not be the preferred implementation of the standard. (If you want to keep the two consistent, perhaps neither would be writable. But personally, I don't think it would be so surprising for the imaginary part of a complex array to be writable while the imaginary part of a real array is not. After all, a complex value cannot be assigned to an element of a real array.) That said, I haven't yet identified a need for |
The latest version of torch does allow >>> torch.real(torch.tensor([1.]))
tensor([1.])
>>> torch.conj(torch.tensor([1.]))
tensor([1.])
>>> torch.imag(torch.tensor([1.]))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
RuntimeError: imag is not implemented for tensors with non-complex dtypes. We agreed at the meeting today that it makes sense to allow real dtype inputs to |
FWIW generally agree and am sympathetic to the arguments Matt makes here. Also agree with Ralf that
While this is possible, have seen NumPy's masked array also take this approach ( |
real
, imag
, and conj
be complex-valuedreal
, imag
, and conj
be complex-valued
gh-427 summarizes the rationale for requiring that inputs to
real
,imag
, andconj
be complex-valued:#446 (comment) states that this is open to being revisited.
If the concern is ambiguity between intended and unintended use, was there a similar discussion for functions that are no-ops for integers, such as
round
andisinf
? It looks like the initial commit that added rounding functions already included the special case for integers, but I couldn't find a discussion about it.Perhaps it didn't come up because it is common for code paths that work on floating point dtypes to also be used for integer dtypes. In the same way, I'd suggest that code for complex dtypes can also be valid for real dtypes. If it is not easy to tell whether unintended or intended uses of these functions on real data is more common in the wild, perhaps the precedent set by NumPy and other libraries (e.g. CuPy, JAX, PyTorch, Dask Array, Tensorflow) is not wrong, and these functions can allow real input.
The text was updated successfully, but these errors were encountered: