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

Make complex sin and cos type stable #13539

Merged
merged 1 commit into from
Oct 13, 2015
Merged

Conversation

malmaud
Copy link
Contributor

@malmaud malmaud commented Oct 11, 2015

cf #11839

@yuyichao
Copy link
Contributor

Would it be too much to add an specialization for Complex{T<:Integer} that bypass the check for infinity and nan?

@tkelman
Copy link
Contributor

tkelman commented Oct 11, 2015

Could use a few @inferred tests

@malmaud
Copy link
Contributor Author

malmaud commented Oct 11, 2015

@tkelman Done
@yuyichao What's the advantage? I'm just following @andreasnoack 's suggestion in the linked issue.

@yuyichao
Copy link
Contributor

Performance? (This is why I didn't go ahead and implement that).

See my benchmark result here

Depending on the machine I use, I can see on average 1%-20% performance improvement for Int and ~1-2% performance improvement (as well as 1% reduction in allocation) for BigInt.

@malmaud
Copy link
Contributor Author

malmaud commented Oct 11, 2015

@yuyichao What do you think of this solution?

@yuyichao
Copy link
Contributor

LGTM as long as there AbstractFloat is the only type that can have infinity / nan....

I've also noticed that the speed up I've seen for the Int version might be related to #13551 (The results are very different with #13553 and the version that has special treatment for integers can be ~10% slower in some cases).

It's probably better to benchmark different versions and pick the best one.

@malmaud
Copy link
Contributor Author

malmaud commented Oct 11, 2015

Hmm, maybe it would be more generic to reverse things and have the generic version check for infinity and nan and an int-specialized version bypass those checks.

@ViralBShah ViralBShah added the maths Mathematical functions label Oct 12, 2015
@malmaud malmaud force-pushed the jmm/complex_trig_stable branch from c0eb095 to 0792585 Compare October 12, 2015 17:48
@malmaud
Copy link
Contributor Author

malmaud commented Oct 12, 2015

Alright, this is g2g.

jakebolewski added a commit that referenced this pull request Oct 13, 2015
@jakebolewski jakebolewski merged commit 7d536e4 into master Oct 13, 2015
@jakebolewski jakebolewski deleted the jmm/complex_trig_stable branch October 13, 2015 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants