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

Add complex number support to round #440

Merged
merged 2 commits into from
Jul 7, 2022
Merged

Add complex number support to round #440

merged 2 commits into from
Jul 7, 2022

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented May 23, 2022

This PR

  • adds complex number support to round. Namely, when applying round to complex number operands, the real and imaginary components must be independently rounded.

Notes

  • Currently, array libraries generally only support rounding complex numbers in round, but not ceil, floor, or trunc. Reasons for not extending complex number support to floor, ceil, and trunc are discussed in a comment below.

@kgryte kgryte added API change Changes to existing functions or objects in the API. topic: Complex Data Types Complex number data types. labels May 23, 2022
@kgryte kgryte added this to the v2022 milestone May 23, 2022
@rgommers
Copy link
Member

Is this a good idea? NumPy and PyTorch both don't support this, and I'm guessing other libraries do too. Is this unambiguously defined in C99? The issue just mentions Matlab

@kgryte
Copy link
Contributor Author

kgryte commented May 23, 2022

C99 doesn't expose rounding functions in its complex number arithmetic. Presumably this is because, for an individual complex number, relatively trivial to round the components separately.

NumPy does independent rounding, but only for round. Seems odd, to me at least, that one would support component rounding for one API, but not the other functions. The principal idea is the same.

If we are going to not provide APIs for independently rounding components, we should probably have a rationale for a lack of explicit support in the standard. From a performance perspective, for those libraries implementing complex number arrays as interleaved strided arrays, cache locality will be better if only a single function call is needed for round components rather than require (round(real(x)) and round(imag(x))).

@rgommers
Copy link
Member

If we are going to not provide APIs for independently rounding components, we should probably have a rationale for a lack of explicit support in the standard

I'd say "lack of support in array libraries" is a good rationale. If anyone cares, they should go implement it first. Maybe we can provide guidance like "if you implement this, do it separate" (reserved functionality, like in a few other places). But requiring a thing that apparently no one has a need for feels a little odd.

@kgryte
Copy link
Contributor Author

kgryte commented May 30, 2022

...that apparently no one has a need for feels a little odd.

Not clear that this is the case. For example, PyTorch previously had support for complex floor, ceil, and trunc (see pytorch/pytorch#35592); however, this was disabled in order to be compatible with NumPy. For NumPy, someone opened an issue concerning complex number support in np.trunc (see numpy/numpy#17100); however, this did not move forward due to concerns about whether NumPy should be consistent with Python.

I recognize that a guiding principle of the consortium is giving adequate weight to precedent when making specification decisions; however, we should also be cognizant as to whether simply because something is the case, it ought to be the case.

For example, NumPy's behavior could be due to around/round_ not leveraging the exact same implementation machinery as floor, ceil, and trunc (e.g., ufuncs vs specialized needs due to additional arguments in around). And the fact that most array libraries follow suit could be due to NumPy's limitations or historical accident, rather than the result of intentional design (as we've seen with other APIs). Now, this may not be the case, but it certainly gives me pause when I'd expect that, if one can round a complex number, one should be able to perform other rounding-like ops on a complex number--something supported in other languages such as MATLAB and Mathematica.

Now, as it turns out, there are legitimate reasons to not add complex number support to floor, ceil, and trunc in terms of independently flooring/ceiling/truncating real and imaginary components. Reasons have been discussed in the Julia community (see JuliaLang/julia#8291 and JuliaLang/julia#42060).

Namely, extending floor (and by extension ceil and trunc) to complex numbers is non-trivial due to certain properties which need to be upheld (see https://www.jsoftware.com/papers/eem/complexfloor.htm for further discussion). Independently flooring real and imaginary components violates the principal that the magnitude of the difference of a number and its floor should be less than unity.

Given that the APL implementation of complex floor (and by association ceil and trunc) has not been implemented by array libraries, seems reasonable to not include their support in the specification at this time. And realistically, given that extending floor/ceil/trunc to the complex domain is likely to result in something which is unintuitive to most users (as most users would probably expect round, floor, ceil, and trunc to all work similarly), seems prudent to not add support in the future.

Instead, users should be expected to be explicit and intentional when attempting to floor/ceil/trunc complex numbers. This is not without performance costs for users of libraries such as NumPy, but better to push responsibility for resolving implementation ambiguity to userland.

Accordingly, I have updated the PR to drop complex number support for ceil, floor, and trunc. However, complex number support in round remains.

@kgryte kgryte changed the title Add complex number support to round, ceil, floor, and trunc Add complex number support to round May 30, 2022
@leofang
Copy link
Contributor

leofang commented Jun 6, 2022

I don't know why anyone would want to round a complex number, do we have any particular use case?

@kgryte
Copy link
Contributor Author

kgryte commented Jun 23, 2022

Based on the most recent consortium meeting (2022-06-23), this PR can move forward. If adding complex number support to floor, ceil, and trunc is desirable in the future, we can revisit then.

@kgryte
Copy link
Contributor Author

kgryte commented Jul 7, 2022

As this PR has been discussed and preserves the status quo across array libraries, will merge.

@kgryte kgryte merged commit 596ef94 into main Jul 7, 2022
@kgryte kgryte deleted the cmplx-round branch July 7, 2022 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change Changes to existing functions or objects in the API. topic: Complex Data Types Complex number data types.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants