Add support for precomputed kernel. #73

barucden · 2021-06-09T16:07:55Z

This is an effort to properly support precomputed kernel.

There are few more steps left:

Closes #71

codecov-commenter · 2021-06-09T16:11:43Z

Codecov Report

Merging #73 (852b2c6) into master (56e9a2e) will increase coverage by 1.86%.
The diff coverage is 98.46%.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   84.75%   86.61%   +1.86%     
==========================================
  Files           5        6       +1     
  Lines         223      254      +31     
==========================================
+ Hits          189      220      +31     
  Misses         34       34

Impacted Files	Coverage Δ
src/LIBSVM.jl	`93.24% <94.11%> (-0.70%)`	⬇️
src/nodes.jl	`100.00% <100.00%> (ø)`

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 56e9a2e...852b2c6. Read the comment docs.

src/LIBSVM.jl

iblislin · 2021-06-09T17:53:50Z

Are there any sample code of precomputed kernel in C that can help us for verifying these changes?

barucden · 2021-06-10T08:59:50Z

Are there any sample code of precomputed kernel in C that can help us for verifying these changes?

There are some tests in scikit learn. The test that I have added is loosely based on it. When I passed the test-case problem to svm-train (I mean the CLI program from LIBSVM) directly, I received different results than those in scikit. In scikit, the indices of the support vectors are [1, 3], while I saw [2, 4]. I believe this is because LIBSVM uses one-based indexing, and scikit transforms that to zero-based (did not verify though). Next, coefs are differently ordered in scikit ([[-0.25, .25]] vs our [0.25; -0.25]).

Anyway, I think we can use svm-train to produce expected results for our test cases.

barucden · 2021-06-10T16:44:59Z

Prediction now works (according to the simple test case, at least).

barucden · 2021-06-11T09:12:41Z

I have added two more test cases. I think we can consider it working.

src/LIBSVM.jl

This commit is the necessary minimum to support precomputed kernel. I tried to keep the number of changes low, so the code could probably use some polishing. I also added a basic test (the expected values are based on the output of `svm-train` from LIBSVM).

We now throw `DimensionMismatch` instead of `ArgumentError` in case of invalid shape of the input matrix.

The prediction with precomputed kernel now works. Although, the code gets uglier and uglier.

It also fixes getting the number of instances for prediction in the case of precomputed kernel.

In the case of precomputed kernel, the first column of nodes must contain indices of the instances. This special method, `gram2nodes`, adds the indices implicitly so we don't have to add them to the input matrix explicitly.

A method is introduced to create `SVMNode`s from ordinary instances or from a precomputed kernel matrix. So far, it only works for full matrices, not sparse ones.

This commit creates a new file `nodes.jl` which contains the code that converts input matrices into `SVMNode`s. Moreover, the function that converts instances represented as a sparse matrix is a bit changed to use only exported functions from SparseArrays. When the matrix containing kernel values is sparse, we convert it to a full matrix and use the full matrix implementation. Hopefully, it should not matter too much as there is a good chance that sparse matrices are not that useful for kernel values.

barucden · 2021-06-15T12:32:36Z

Ok, I guess this is ready for reviews now.

One thing that is not so great is that sparse matrices will be converted to full matrices in the case of precomputed kernel. The reason why I did this is that I could not figure out how to reuse the code that we use for sparse matrices in the case of non-precomputed kernels. I don't think this is a significant issue since kernels tend to produce non-zero values anyway.

mrussek

This looks good to me, but obviously would like another maintainer to review

src/nodes.jl

This commit introduces a constant instance of `SVMNode(-1, NaN)` that is used as every terminal `SVMNode`.

iblislin · 2021-06-16T12:07:01Z

src/LIBSVM.jl

@@ -334,6 +309,7 @@ function svmtrain(
        weights = Float64[]
        reverse_labels = Bool[]
    else
+        check_train_input(X, y, kernel)


I think EpsilonSVR and NuSVR also need this check?

Good catch, I think so too.

The check was originally in the indices_and_weights so it's been missing for EpsilonSVR and NuSVR for quite some time. Should I add it?

yeah, adding that check in this PR is fine for me.

iblislin · 2021-06-16T16:16:49Z

@barucden I'm going to make a new release for this changes. Are there any planing PRs desired to be shipped with the new release?

ablaom · 2021-06-17T01:18:59Z

@barucden Thank you indeed for this work. I realize it was more than you bargained for.

While it's fresh in your mind, could you add some brief clue in the readme documentation on the new option to specify the kernel? That will expedite updating the MLJ interface.

barucden · 2021-06-17T08:38:21Z

@barucden I'm going to make a new release for this changes. Are there any planing PRs desired to be shipped with the new release?

We could address #61 in this release if you are interested. I still have the code.

@barucden Thank you indeed for this work. I realize it was more than you bargained for.

While it's fresh in your mind, could you add some brief clue in the readme documentation on the new option to specify the kernel? That will expedite updating the MLJ interface.

I added a few notes to the docs of the relevant methods (svmtrain, svmpredict) already, but I agree that a small example that shows the whole thing might help too.

barucden marked this pull request as draft June 9, 2021 16:08

barucden mentioned this pull request Jun 9, 2021

How does one specify a pre-computed kernel? #71

Closed

iblislin reviewed Jun 9, 2021

View reviewed changes

src/LIBSVM.jl Outdated Show resolved Hide resolved

iblislin reviewed Jun 9, 2021

View reviewed changes

src/LIBSVM.jl Outdated Show resolved Hide resolved

barucden force-pushed the precomputed-kernel branch from a3dee33 to 7ae09ed Compare June 10, 2021 16:43

barucden force-pushed the precomputed-kernel branch 3 times, most recently from e105f0d to d0ddc4c Compare June 11, 2021 09:02

iblislin reviewed Jun 12, 2021

View reviewed changes

src/LIBSVM.jl Outdated Show resolved Hide resolved

barucden added 7 commits June 12, 2021 20:09

Change ArgumentError to DimensionMismatch.

7828f7e

We now throw `DimensionMismatch` instead of `ArgumentError` in case of invalid shape of the input matrix.

Adjust svmpredict to support precomputed kernel.

b9341b0

The prediction with precomputed kernel now works. Although, the code gets uglier and uglier.

Add more difficult test case.

3b3bf29

It also fixes getting the number of instances for prediction in the case of precomputed kernel.

Add a test case with Iris data.

ccf9c4d

Add a test-case with a sparse input matrix

8379bdc

Start indexing SVMNodes from 1

9ae6fac

barucden force-pushed the precomputed-kernel branch 2 times, most recently from 3a92dbf to a9fb62e Compare June 14, 2021 17:01

barucden added 2 commits June 14, 2021 19:01

Precomp. kernel: special method for creating nodes

a9fb62e

In the case of precomputed kernel, the first column of nodes must contain indices of the instances. This special method, `gram2nodes`, adds the indices implicitly so we don't have to add them to the input matrix explicitly.

Unify conversion to nodes

f53aef9

A method is introduced to create `SVMNode`s from ordinary instances or from a precomputed kernel matrix. So far, it only works for full matrices, not sparse ones.

barucden force-pushed the precomputed-kernel branch from bc93253 to 498c1d9 Compare June 15, 2021 12:00

barucden added 2 commits June 15, 2021 14:00

Add docs for precomputed kernel

294b75e

barucden marked this pull request as ready for review June 15, 2021 12:27

mrussek reviewed Jun 16, 2021

View reviewed changes

iblislin reviewed Jun 16, 2021

View reviewed changes

src/nodes.jl Outdated Show resolved Hide resolved

iblislin reviewed Jun 16, 2021

View reviewed changes

src/nodes.jl Show resolved Hide resolved

barucden added 2 commits June 16, 2021 12:34

Use a single instance for all terminal SVMNodes

8f24b09

This commit introduces a constant instance of `SVMNode(-1, NaN)` that is used as every terminal `SVMNode`.

Remove unnecessary using

6048e42

iblislin reviewed Jun 16, 2021

View reviewed changes

Add input check for regression machines

852b2c6

iblislin merged commit 5083799 into JuliaML:master Jun 16, 2021

barucden deleted the precomputed-kernel branch June 17, 2021 08:38

barucden mentioned this pull request Jun 17, 2021

README: documentation for precomputed kernel #83

Merged

ablaom mentioned this pull request Jun 17, 2021

Add interface point to specify precomputed kernel JuliaAI/MLJClusteringInterface.jl#8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for precomputed kernel. #73

Add support for precomputed kernel. #73

barucden commented Jun 9, 2021 •

edited

Loading

codecov-commenter commented Jun 9, 2021 •

edited

Loading

iblislin commented Jun 9, 2021

barucden commented Jun 10, 2021

barucden commented Jun 10, 2021

barucden commented Jun 11, 2021

barucden commented Jun 15, 2021

mrussek left a comment

iblislin Jun 16, 2021

barucden Jun 16, 2021

iblislin Jun 16, 2021 •

edited

Loading

barucden Jun 16, 2021

iblislin commented Jun 16, 2021

ablaom commented Jun 17, 2021

barucden commented Jun 17, 2021

Add support for precomputed kernel. #73

Add support for precomputed kernel. #73

Conversation

barucden commented Jun 9, 2021 • edited Loading

codecov-commenter commented Jun 9, 2021 • edited Loading

Codecov Report

iblislin commented Jun 9, 2021

barucden commented Jun 10, 2021

barucden commented Jun 10, 2021

barucden commented Jun 11, 2021

barucden commented Jun 15, 2021

mrussek left a comment

Choose a reason for hiding this comment

iblislin Jun 16, 2021

Choose a reason for hiding this comment

barucden Jun 16, 2021

Choose a reason for hiding this comment

iblislin Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

barucden Jun 16, 2021

Choose a reason for hiding this comment

iblislin commented Jun 16, 2021

ablaom commented Jun 17, 2021

barucden commented Jun 17, 2021

barucden commented Jun 9, 2021 •

edited

Loading

codecov-commenter commented Jun 9, 2021 •

edited

Loading

iblislin Jun 16, 2021 •

edited

Loading