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

Complex.Sqrt comply with IEEE 754 and use double.Hypot #104262

Merged
merged 5 commits into from
Jul 3, 2024

Conversation

lilinus
Copy link
Contributor

@lilinus lilinus commented Jul 1, 2024

Contributes to #104233

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 1, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

@lilinus lilinus marked this pull request as ready for review July 1, 2024 20:54
@tannergooding
Copy link
Member

Looks like there's some test failures caused by using Hypot.

In particular Complex.Abs for Complex(NaN, -inf) is saying it expects NaN, but double.Hypot produces +inf instead. The behavior of double.Hypot strictly matches the IEEE 754 rules which dictate:

For the hypot operation, hypot(±0, ±0) is +0, hypot(±∞, qNaN) is +∞, and hypot(qNaN, ±∞) is +∞.

The C language specification (which is typically considered the source of truth for IEEE 754 compliance of Complex arithmetic) in G.6 Complex artihmetic dictates

Each of the functions cabs and carg is specified by a formula in terms of a real function (whose special cases are covered in Annex F):

cabs(x + iy) = hypot(x, y)
carg(x + iy) = atan2(y, x)

Where Annex F then matches the IEEE 754 rules, thus the new behavior is strictly correct and is fixing a latent bug in this edge case.


There is a similar failure for Complex.Sqrt given Complex(-inf, NaN) where it is expecting the imaginary result to be NaN but the actual result is +inf

G.6.4.2 The csqrt functions in the C language specification dictates:

— csqrt(conj(z)) = conj(csqrt(z)).
— csqrt(±0 + i0) returns +0 + i0.
— csqrt(x + i∞) returns +∞+ i∞, for all x (including NaN).
— csqrt(x + iNaN) returns NaN + iNaN and optionally raises the "invalid" floating-point exception, for finite x.
— csqrt(−∞ + iy) returns +0 + i∞, for finite positive-signed y.
— csqrt(+∞+ iy) returns +∞+ i0, for finite positive-signed y.
— csqrt(−∞ + iNaN) returns NaN±i∞ (where the sign of the imaginary part of the result is unspecified).
— csqrt(+∞+ iNaN) returns +∞+ iNaN.
— csqrt(NaN + iy) returns NaN + iNaN and optionally raises the "invalid" floating-point exception, for finite y.
— csqrt(NaN + iNaN) returns NaN + iNaN.

And thus this is another case where the new behavior is strictly more correct. There's one other edge here that appears to not be handled correctly as well, but which aren't failing the tests. In particular Complex(NaN, +inf) should be producing Complex(+inf, +inf) but is actually producing Complex(NaN, NaN)

@lilinus lilinus changed the title Use double.Hypot in Complex Complex.Sqrt comply with IEEE 754 and use double.Hypot Jul 2, 2024
@tannergooding tannergooding merged commit 898ffc5 into dotnet:main Jul 3, 2024
83 checks passed
@lilinus lilinus deleted the use-hypot-in-complex branch July 18, 2024 08:41
@github-actions github-actions bot locked and limited conversation to collaborators Aug 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Numerics community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants