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

Refactor var/varm #17

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Refactor var/varm #17

wants to merge 2 commits into from

Conversation

yuehhua
Copy link

@yuehhua yuehhua commented Dec 12, 2019

Try to clarify the dependency between interfaces and implementations.
This also bring a little bit improvement in performance.

@yuehhua
Copy link
Author

yuehhua commented Dec 12, 2019

Test original benchmark in v1.3

julia> using Statistics

julia> using BenchmarkTools

julia> A = rand(10000);

julia> @benchmark var($A; mean=nothing)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     4.842 μs (0.00% GC)
  median time:      5.291 μs (0.00% GC)
  mean time:        5.727 μs (0.00% GC)
  maximum time:     19.203 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     7

julia> @benchmark var($A; mean=0.1)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     4.864 μs (0.00% GC)
  median time:      4.953 μs (0.00% GC)
  mean time:        5.280 μs (0.00% GC)
  maximum time:     12.545 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     7

julia> @benchmark varm($A, 0.1)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     2.670 μs (0.00% GC)
  median time:      2.878 μs (0.00% GC)
  mean time:        3.072 μs (0.00% GC)
  maximum time:     8.599 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     9

@yuehhua
Copy link
Author

yuehhua commented Dec 12, 2019

Benchmark after refactor

julia> using Statistics

julia> using BenchmarkTools

julia> A = rand(10000);

julia> @benchmark var($A; mean=nothing)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     4.881 μs (0.00% GC)
  median time:      4.908 μs (0.00% GC)
  mean time:        5.081 μs (0.00% GC)
  maximum time:     11.873 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     7

julia> @benchmark var($A; mean=0.1)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     4.882 μs (0.00% GC)
  median time:      4.910 μs (0.00% GC)
  mean time:        5.113 μs (0.00% GC)
  maximum time:     10.920 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     7

julia> @benchmark varm($A, 0.1)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     2.627 μs (0.00% GC)
  median time:      2.647 μs (0.00% GC)
  mean time:        2.763 μs (0.00% GC)
  maximum time:     7.039 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     9

src/Statistics.jl Outdated Show resolved Hide resolved
src/Statistics.jl Outdated Show resolved Hide resolved
@nalimilan
Copy link
Member

Thanks. But do you have an idea of the behavior in terms of precision of the old and new algorithms when mean=nothing? Since there's virtually no performance improvement, we need strong reasons to move from one algorithm to the other. And why would we keep varm using one algorithm and var another one when mean !=nothing?

@mschauer
Copy link
Member

Should we separate the question of which algorithm is called in the end from the general cleanup in this PR which is nice I believe?

@yuehhua
Copy link
Author

yuehhua commented Jan 24, 2020

Sorry for not following up this PR due to my busy time. This is still not much performance improvement but API improvement. Thanks for @mschauer's support. As for algorithm improvement, if I have time to address this issue, I will have another PR in the future.

@ViralBShah ViralBShah marked this pull request as draft October 20, 2024 14:34
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