JuliaML / LIBSVM.jl

LIBSVM bindings for Julia
Other
88 stars 35 forks source link

refine svmpredict #77

Closed iblislin closed 3 years ago

iblislin commented 3 years ago

Ref: https://github.com/JuliaML/LIBSVM.jl/pull/76#discussion_r650039472

~I imported the function libsvm_predict_probability and libsvm_predict_values for temporary and discussion needs. They should be relocated later in #76.~ Rebased.

codecov-commenter commented 3 years ago

Codecov Report

Merging #77 (3a9695c) into master (76e57ad) will increase coverage by 1.20%. The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
+ Coverage   82.88%   84.09%   +1.20%     
==========================================
  Files           5        5              
  Lines         222      220       -2     
==========================================
+ Hits          184      185       +1     
+ Misses         38       35       -3     
Impacted Files Coverage Δ
src/libcalls.jl 85.71% <50.00%> (ø)
src/LIBSVM.jl 93.20% <91.66%> (+1.74%) :arrow_up:

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 76e57ad...3a9695c. Read the comment docs.

barucden commented 3 years ago

It would be great if we managed to make svmpredict more type-stable. Currently, the type of pred is uncertain, as well as output (before your commit). Perhaps we could define some auxiliary functions as they do here? I believe it would result not only in more performant but also cleaner code.

iblislin commented 3 years ago

It would be great if we managed to make svmpredict more type-stable.

I added svmpredict_fill!, it became slightly faster.

Before:

julia> A = rand(2, 2_000_000);

julia> @benchmark svmpredict($model, $A)
BenchmarkTools.Trial:
  memory estimate:  152.59 MiB
  allocs estimate:  15
  --------------
  minimum time:     1.185 s (0.10% GC)
  median time:      1.207 s (1.83% GC)
  mean time:        1.236 s (3.57% GC)
  maximum time:     1.305 s (7.68% GC)
  --------------
  samples:          5
  evals/sample:     1

After:

julia> @benchmark svmpredict($model, $A)
BenchmarkTools.Trial:
  memory estimate:  152.59 MiB
  allocs estimate:  16
  --------------
  minimum time:     1.062 s (0.11% GC)
  median time:      1.092 s (2.34% GC)
  mean time:        1.113 s (4.13% GC)
  maximum time:     1.195 s (8.72% GC)
  --------------
  samples:          5
  evals/sample:     1
iblislin commented 3 years ago

Given that there are some CI failures popped out randomly, I suspect that there are some GC safety issues

e.g. https://github.com/JuliaML/LIBSVM.jl/pull/77/checks?check_run_id=2807830407

iblislin commented 3 years ago

@barucden good to go?

barucden commented 3 years ago

@barucden good to go?

I am trying to replicate the issue caught by one of the tests. So far I did not succeed.

iblislin commented 3 years ago

I cannot reproduce that on my machine either.

I can add some GC.@preserve in other PR and let's see the random error pop out in future or not.

barucden commented 3 years ago

Alright :+1: