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 sintau/costau consistent with sinpi/cospi #19

Merged
merged 1 commit into from
May 14, 2017
Merged

Make sintau/costau consistent with sinpi/cospi #19

merged 1 commit into from
May 14, 2017

Conversation

giordano
Copy link
Member

@giordano giordano commented May 9, 2017

julia> sin(NaN)
NaN

julia> sin(Inf)
ERROR: DomainError:
Stacktrace:
 [1] nan_dom_err at ./math.jl:299 [inlined]
 [2] sin(::Float64) at ./math.jl:416

julia> sintau(NaN)
NaN

julia> sintau(Inf)
ERROR: DomainError:
Stacktrace:
 [1] sintau(::Float64) at /home/mose/.julia/v0.7/Tau/src/trig.jl:5

Note that this PR makes the test fail on Julia 0.4 (see #13) because in Julia 0.4:

julia> sin(complex(Inf, Inf))
Inf + NaN*im

while in Julia 0.5 and later

julia> sin(complex(Inf, Inf))
NaN + Inf*im

I also changed the first test to

@test tau  2 * pi

because it's exactly the fact that tau isn't the same as 2 * pi that makes this package particularly useful.

@codecov-io
Copy link

codecov-io commented May 9, 2017

Codecov Report

Merging #19 into master will increase coverage by 5.55%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #19      +/-   ##
========================================
+ Coverage   94.44%   100%   +5.55%     
========================================
  Files           2      2              
  Lines          54     61       +7     
========================================
+ Hits           51     61      +10     
+ Misses          3      0       -3
Impacted Files Coverage Δ
src/trig.jl 100% <100%> (+5.66%) ⬆️

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 341bf21...f207e85. Read the comment docs.

@giordano giordano changed the title Make sintau/costau consistent with sin/cos [WIP] Make sintau/costau consistent with sin/cos May 9, 2017
@giordano giordano changed the title [WIP] Make sintau/costau consistent with sin/cos Make sintau/costau consistent with sinpi/cospi May 9, 2017
@waldyrious
Copy link
Collaborator

Thanks for doing this! I should be able to do a proper review in the weekend.

@giordano
Copy link
Member Author

I added more extensive tests. I always compare the results with sinpi/cospi, rather that with sin/cos, because the formers are more accurate.

For the record, the new methods of sintau/costau with complex arguments are often faster than before. Before the PR:

julia> z = complex(0.0, 0.0)
0.0 + 0.0im

julia> @benchmark sintau($z)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     34.275 ns (0.00% GC)
  median time:      36.908 ns (0.00% GC)
  mean time:        37.036 ns (0.00% GC)
  maximum time:     87.059 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     993

julia> z = complex(-1.0, 1.0)
-1.0 + 1.0im

julia> @benchmark sintau($z)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     87.628 ns (0.00% GC)
  median time:      87.806 ns (0.00% GC)
  mean time:        89.831 ns (0.00% GC)
  maximum time:     208.251 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     963

julia> z = complex(3.5, -2.7)
3.5 - 2.7im

julia> @benchmark sintau($z)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     121.915 ns (0.00% GC)
  median time:      122.128 ns (0.00% GC)
  mean time:        123.617 ns (0.00% GC)
  maximum time:     346.113 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     909

After the PR:

julia> z = complex(0.0, 0.0)
0.0 + 0.0im

julia> @benchmark sintau($z)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     10.041 ns (0.00% GC)
  median time:      10.047 ns (0.00% GC)
  mean time:        10.090 ns (0.00% GC)
  maximum time:     29.297 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     999

julia> z = complex(-1.0, 1.0)
-1.0 + 1.0im

julia> @benchmark sintau($z)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     35.591 ns (0.00% GC)
  median time:      35.624 ns (0.00% GC)
  mean time:        35.943 ns (0.00% GC)
  maximum time:     71.093 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     993

julia> z = complex(3.5, -2.7)
3.5 - 2.7im

julia> @benchmark sintau($z)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     118.415 ns (0.00% GC)
  median time:      118.493 ns (0.00% GC)
  mean time:        120.525 ns (0.00% GC)
  maximum time:     226.772 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     911

I believe that there is much room for improvement in the sintau/costau methods with real arguments. They are based on old implementations of the sinpi/cospi functions, the new ones are way faster. However, adapting them to sintau/costau goes beyond my abilities.

Adapt implementations of complex sintau/costau from those of sinpi/cospi in
Julia Standard Library.
@waldyrious
Copy link
Collaborator

LGTM. Nice that the test coverage has reached 100% :)

I would like to discuss the @test tau ≠ 2 * pi change, but that is a separate issue.

@waldyrious waldyrious merged commit 5aa1c5c into JuliaMath:master May 14, 2017
@giordano giordano deleted the sintau-costau branch May 14, 2017 17:15
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.

3 participants