Adapted complex exp() function so that it can handle inf and nan arguments as well #104
Conversation
Could you please add that reference link as a comment in the test? |
…webpage from which the test values were taken.
OK. Done. |
|
A gentle reminder... |
src/lib.rs
Outdated
| // formula: e^(a + bi) = e^a (cos(b) + i*sin(b)) = from_polar(e^a, b) | ||
|
|
||
| // Treat the corner cases +∞, -∞, and NaN | ||
| let mut im = self.im; |
There was a problem hiding this comment.
I think this may be easier to follow if we unpack both parts, and then use them directly throughout.
| let mut im = self.im; | |
| let Complex { re, mut im } = self; |
There was a problem hiding this comment.
Thanks for the suggestion, I just implemented, tested, and git-pushed it.
src/lib.rs
Outdated
| if self.re.is_infinite() { | ||
| if self.re < T::zero() { | ||
| if !self.im.is_finite() { | ||
| im = T::one(); |
There was a problem hiding this comment.
Where does this 1.0 come from -- is that an arbitrary choice?
I think the result will be 0+0i, so why not just return that?
There was a problem hiding this comment.
It's indeed an arbitrary choice (following the clang C++ implementation). I agree that it would be simpler to just return 0+0i, so I just implemented, tested, and git-pushed that.
src/lib.rs
Outdated
| // Compare the imaginary parts | ||
| if a.im.is_finite() { | ||
| if b.im.is_finite() { | ||
| close = (a.im == b.im) || (a.im - b.im).abs() < tol; |
There was a problem hiding this comment.
We shouldn't lose the re comparison.
| close = (a.im == b.im) || (a.im - b.im).abs() < tol; | |
| close &= (a.im == b.im) || (a.im - b.im).abs() < tol; |
There was a problem hiding this comment.
Aargh, sorry about that... None of the unit tests caught this. I just fixed the bug.
There was a problem hiding this comment.
Yeah, "Who tests the tester?" It could just return true, and the unit tests would pass. :)
|
bors r+ |
|
Build succeeded: |
Why this PR?
The current version of the complex exp() function is not able to handle arguments that contain +/- inf or NaN in their real or imaginary part. This impacts other complex functions that use the exp() function.
For example, for the most widely used implementation of the complex Faddeeva function
w(), the currentexp()implementation leads tow(1e160 - 1e159*i) = NaN + NaN *i, while the correct value is-5.586035480670854e-162 + 5.5860354806708545e-161 * i. The underlying reason is that the currentexp()implementation erroneously returnsexp(-inf + inf *i) = NaN + Nan *iinstead of the correct0 + 0*i.Cf also issue #103.
Contents of this PR
<complex>C++ header that comes with clang++.close_naninf()andclose_naninf_to_tol()as the existing functionsclose()andclose_to_tol()are not able to deal with inf and nan.