-
Notifications
You must be signed in to change notification settings - Fork 71
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
Speed up sin and cos by a factor 6.5 #117
Conversation
72ea4dd
to
b7b1d03
Compare
I have not found a sensible way to do tan correctly. Hence leaving tan aside for now. |
Is this ready for review? Ping me when it is ready. (Regarding |
Yes, ready for review. |
Codecov Report
@@ Coverage Diff @@
## master #117 +/- ##
==========================================
- Coverage 92.81% 92.65% -0.16%
==========================================
Files 21 21
Lines 960 967 +7
==========================================
+ Hits 891 896 +5
- Misses 69 71 +2
Continue to review full report at Codecov.
|
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 have submitted two optional suggestions; I think this can be merged.
Multiply an interval by a positive constant. | ||
For efficiency, does not check that the constant is positive. | ||
""" | ||
multiply_by_positive_constant(α, x::Interval) = @round(α*x.lo, α*x.hi) |
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.
Perhaps instead of defining multiply_by_positive_constant
we should rather define the methods *(a::T, x::Interval)
and *(x::Interval, a::T)
(in arithmetic.jl
), where T<:Real
(or maybe <:Number
), or even unspecified, as it is here. Currently we use promotion to an interval and rounding, which may slow down things.
Question: Below you use this function for to define half_pi(::Type{Float64})
, but not for other types; can you explain why?
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 use it for two_pi
as well.
Actually the half_pi
function is no longer used, I believe.
We may well want to define explicit methods for *(constant, Interval)
. I've opened an issue: #120
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.
That was actually the suggestion.
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.
Right, but I'd prefer to leave it for a separate PR.
Also, if you actually know that the constant is positive then this specialised version is even slightly quicker.
temp = atomic(Interval{T}, x) / half_pi(x) | ||
(floor(temp.lo), floor(temp.hi)) | ||
|
||
return SVector(floor(temp.lo), floor(temp.hi)) |
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.
What about returning a tuple here, instead of a SVector
? (Same comment applies in line 44.)
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 used to be a tuple; changing it to an SVector
had a ridiculously big impact on the speed due to the minimum
and maximum
below that are not specialised for tuples!
In any case all of this code can be simplified a lot when #119 is possible, I believe.
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.
Fair enough.
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.
OK, I found an alternative solution: return a tuple and do
max(tuple...)
Too late...
Current master:
This PR: