Skip to content

define sin/cos on Dual to use sincos#561

Merged
KristofferC merged 1 commit intomasterfrom
kc/sincos
Nov 22, 2021
Merged

define sin/cos on Dual to use sincos#561
KristofferC merged 1 commit intomasterfrom
kc/sincos

Conversation

@KristofferC
Copy link
Collaborator

A bit more efficient since sincos can share some computation between the sin and cos:

PR:

julia> d = Dual(1.0,2.0,3.0,4.0,5.0)

julia> @btime sin($d)
  10.050 ns (0 allocations: 0 bytes)
Dual{Nothing}(0.8414709848078965,1.0806046117362795,1.6209069176044193,2.161209223472559,2.701511529340699)

Master:

julia> d = Dual(1.0,2.0,3.0,4.0,5.0)

julia> @btime sin($d)
  13.638 ns (0 allocations: 0 bytes)
Dual{Nothing}(0.8414709848078965,1.0806046117362795,1.6209069176044193,2.161209223472559,2.701511529340699)

@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2021

Codecov Report

Merging #561 (402ae93) into master (d033d2a) will increase coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #561      +/-   ##
==========================================
+ Coverage   84.95%   85.11%   +0.15%     
==========================================
  Files           9        9              
  Lines         844      853       +9     
==========================================
+ Hits          717      726       +9     
  Misses        127      127              
Impacted Files Coverage Δ
src/dual.jl 73.55% <100.00%> (+0.45%) ⬆️
src/jacobian.jl 99.31% <0.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d033d2a...402ae93. Read the comment docs.

return Dual{T}(s, c * partials(d))
end

@inline sincos(x) = (sin(x), cos(x))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know why this was here.

Copy link
Contributor

@chriselrod chriselrod left a comment

Choose a reason for hiding this comment

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

Might be worth doing similar for sincosd, at least if isdefined(Base, :sincos)?

@KristofferC
Copy link
Collaborator Author

Might be worth doing similar for sincosd

Base defines sincosd(x) as sind(x), cosd(x) so it doesn't seem there is any performance to gain there.

@KristofferC KristofferC merged commit d551bbe into master Nov 22, 2021
@KristofferC KristofferC deleted the kc/sincos branch November 22, 2021 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants