-
Notifications
You must be signed in to change notification settings - Fork 49
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aargh, sorry about that... None of the unit tests caught this. I just fixed the bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 *i
instead 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.