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.

Sign up for GitHub

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

Jump to bottom

Make expm1(::Complex64) type stable. #21896

Merged
merged 1 commit into from
May 16, 2017
Merged

Make expm1(::Complex64) type stable. #21896

merged 1 commit into from
May 16, 2017

Conversation

yuyichao
Copy link
Contributor

Noticed by accident when debugging #21831 (comment) .... The issue is apparently there ever since expm1(::Complex) is defined in 0.3....

@kshyatt kshyatt added the maths Mathematical functions label May 16, 2017
@JeffBezanson
Copy link
Member

LGTM.

@ararslan ararslan merged commit a34fd51 into master May 16, 2017
@ararslan ararslan deleted the yyc/expm1 branch May 16, 2017 19:58
@ViralBShah
Copy link
Member

Should this be backported to 0.6?

@yuyichao
Copy link
Contributor Author

Ah, probably. I wasn't sure if 0.5 has the necessary float method defined but then also forgot to add the 0.6 backport label...............................

@yuyichao
Copy link
Contributor Author

Actually float(Int) is defined even on 0.4 so I think we can backport to 0.5 if there isn't much conflict.

tkelman pushed a commit that referenced this pull request May 17, 2017
@yuyichao @tkelman
Make expm1(::Complex64) type stable. (#21896)
f445c6a 
(cherry picked from commit a34fd51)
giordano
giordano reviewed May 17, 2017
View reviewed changes
base/complex.jl
wr = isfinite(er) ? erm1 - 2.0*er*(sin(0.5*zi))^2 : er*cos(zi)
Complex(wr, er*sin(zi))
if isfinite(er)
wr = erm1 - 2 * er * (sin(convert(Tf, 0.5) * zi))^2
Copy link
Contributor

@giordano giordano May 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I'm a bit too late, but why not using zi / 2 instead of converting 0.5? The performance difference is sensible for BigFloat for this operation (not the whole function, though).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just trying to not change the result for machine types and being conservative about this transformation. I assume this transformation is exact for Float16, Float32 and Float64 (though floating point corner cases frequently surprises me...)? If that's the case then I think we can change this to / 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giordano giordano giordano left review comments

Assignees
No one assigned
Labels
maths Mathematical functions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yuyichao @JeffBezanson @ViralBShah @giordano @kshyatt @tkelman @ararslan