search

gdkrmr / WeightedOnlineStats.jl

Weighted version of OnlineStats.jl
MIT License
10 stars 4 forks source link

Hist with fixed edges #20

Closed jw3126 closed 5 years ago

jw3126 commented 5 years ago

I am in need for a histogram with edges fixed by the user. Should I provide a PR?

gdkrmr commented 5 years ago

A PR would be great! Could you try to mirror the api from OnlineStats.jl, we are trying to be drop in replacements, if possible.

jw3126 commented 5 years ago

Why is WeightedHist just a wrapper around alg? https://github.com/gdkrmr/WeightedOnlineStats.jl/blob/master/src/histogram.jl#L27

jw3126 commented 5 years ago

Mhh for me tests on fresh master fail:

WeightedCovMatrix fit!: Error During Test at /home/jan/.julia/dev/WeightedOnlineStats/test/test_cov.jl:1
  Got exception outside of a @test
  MethodError: no method matching getindex(::Base.Generator{Base.OneTo{Int64},getfield(Base, Symbol("##175#176")){Array{Float64,2}}}, ::Int64)
  Stacktrace:
   [1] fit!(::WeightedCovMatrix{Float64}, ::Base.Generator{Base.OneTo{Int64},getfield(Base, Symbol("##175#176")){Array{Float64,2}}}, ::Array{Float64,1}) at /home/jan/.julia/dev/WeightedOnlineStats/src/interface.jl:28
   [2] fit!(::WeightedCovMatrix{Float64}, ::Array{Float64,2}, ::Array{Float64,1}) at /home/jan/.julia/dev/WeightedOnlineStats/src/interface.jl:35
   [3] top-level scope at /home/jan/.julia/dev/WeightedOnlineStats/test/test_cov.jl:23
   [4] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.1/Test/src/Test.jl:1083
   [5] top-level scope at /home/jan/.julia/dev/WeightedOnlineStats/test/test_cov.jl:2
   [6] include at ./boot.jl:326 [inlined]
   [7] include_relative(::Module, ::String) at ./loading.jl:1038
   [8] include(::Module, ::String) at ./sysimg.jl:29
   [9] include(::String) at ./client.jl:403
   [10] top-level scope at none:0
   [11] include at ./boot.jl:326 [inlined]
   [12] include_relative(::Module, ::String) at ./loading.jl:1038
   [13] include(::Module, ::String) at ./sysimg.jl:29
   [14] include(::String) at ./client.jl:403
   [15] top-level scope at none:0
   [16] eval(::Module, ::Any) at ./boot.jl:328
   [17] exec_options(::Base.JLOptions) at ./client.jl:243
   [18] _start() at ./client.jl:436
Test Summary:          | Error  Total
WeightedCovMatrix fit! |     1      1
gdkrmr commented 5 years ago

I will fix the failing tests, if you could do the renaming WeightedHist -> WeightedAdaptiveHist it would be great, if not I will do it myself, but this could take a couple of days. The renaming shouldn't be too much work and you avoid name clashes until I fix it myself.

jw3126 commented 5 years ago

How does AdaptiveHist differ from ExpandingHist in OnlineStats? I can do the renaming.

gdkrmr commented 5 years ago

Tests should pass now.

AFAIK, from a quick look at the code, in OnlineStats.ExpandingHist, the edges of the bins are always some kind of regular range. WeightedOnlineStats.Hist fixes the number of bins and and if a point lies outside of the range of the hist, it adds a bin containing the new point and merges two other bin, therefore the bins are not separated by regular intervals and performance is not as good.

gdkrmr commented 5 years ago

Thanks for the contribution!