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

refine svmpredict #77

Merged
merged 6 commits into from
Jun 12, 2021
Merged

refine svmpredict #77

merged 6 commits into from
Jun 12, 2021

Conversation

iblislin
Copy link
Member

@iblislin iblislin commented Jun 11, 2021

Ref: #76 (comment)

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
Copy link

codecov-commenter commented Jun 11, 2021

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%) ⬆️

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

@barucden good to go?

@barucden
Copy link
Member

@barucden good to go?

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

@iblislin
Copy link
Member Author

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
Copy link
Member

Alright 👍

@iblislin iblislin merged commit b018d8e into master Jun 12, 2021
@iblislin iblislin deleted the ib/refine-svmpredict branch June 12, 2021 12:00
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.

None yet

3 participants