more accurate sqrt function #129
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cuviper
reviewed
May 13, 2024
| // Complex64::new(2.4421097261308304e-162, 1.0115549693666347e-162) | ||
| // ); | ||
|
|
||
| if self.re.is_zero() && self.im.is_zero() { |
There was a problem hiding this comment.
Can you add a source for all these special cases? e.g.
https://en.cppreference.com/w/c/numeric/complex/csqrt
(and make sure all those are covered)
| } | ||
| if self.re.is_nan() { | ||
| // nan + nan i | ||
| return Self::new(self.re, (self.im - self.im) / (self.im - self.im)); |
There was a problem hiding this comment.
We have a direct NaN -- not sure if this should also copysign though.
Suggested change
| return Self::new(self.re, (self.im - self.im) / (self.im - self.im)); | |
| return Self::new(self.re, T::nan()); |
| // √(inf +/- x i) = inf +/- 0 i | ||
| // √(-inf +/- NaN i) = NaN +/- inf i | ||
| // √(-inf +/- x i) = 0 +/- inf i | ||
|
|
There was a problem hiding this comment.
Maybe add a variable to make this clearer:
Suggested change
| #[allow(clippy::eq_op)] | |
| let zero_or_nan = self.im - self.im; |
| if scale { | ||
| self = self / four; | ||
| } | ||
| if self.re.is_sign_negative() { |
There was a problem hiding this comment.
We could also use a citation and link in a comment for the algorithm you mentioned.
|
Thanks for the fast review! Apologies for not getting back to this yet. I had to unexpectetly travel for work so I will not be able to get back to this before the weekend. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed that the square root implementation in num-complex uses conversion trough polar coordinates to compute complex squre roots. Usually the algorithm from https://dl.acm.org/doi/abs/10.1145/363717.363780 is used to compute the complex square root instead.
The algorithm uses hypot/norm and square root to compute csqrt. This approach should be faster since less transcendental calls are needed. Hypot and sqrt also tend to be faster compared to exp/ln/atan/cos/sin. Both hypot and sqart also have much higher precision. Most implementations guarantee that these two functions return the correctly rounded infinite accuracy result.
For prior art you can look at the glibc and musl implementation (https://git.musl-libc.org/cgit/musl/tree/src/complex/csqrt.c). The glibc implementation is a lot more complicated/hard to read because they ensure that underflow floating point exceptions are triggered correctly. I don't think that is something num-complex needs to do. There is also some accuracy loss for subnormal numbers that I didn't handle yet. I left a comment about it, but it is very minor.