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

Move to enum Kernels instead of Structs for hypertuning #169

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

montanalow
Copy link
Collaborator

SVCSearchParameters currently uses a Vec for kernels, which means you can't search across multiple kernel types at the same time. This PR converts K to an enum Kernel type, instead of a generic type, so that multiple kinds of kernels may be compared in the same grid search.

@montanalow
Copy link
Collaborator Author

This is a breaking change, so may require more thought on the approach.

@codecov-commenter
Copy link

Codecov Report

Merging #169 (fca9f35) into development (403d3f2) will increase coverage by 0.06%.
The diff coverage is 85.89%.

@@               Coverage Diff               @@
##           development     #169      +/-   ##
===============================================
+ Coverage        84.40%   84.47%   +0.06%     
===============================================
  Files               86       86              
  Lines             9921     9927       +6     
===============================================
+ Hits              8374     8386      +12     
+ Misses            1547     1541       -6     
Impacted Files Coverage Δ
src/model_selection/hyper_tuning.rs 0.00% <ø> (ø)
src/svm/mod.rs 89.47% <84.00%> (+2.98%) ⬆️
src/svm/svr.rs 86.46% <84.61%> (+0.27%) ⬆️
src/svm/svc.rs 88.91% <87.50%> (+1.19%) ⬆️
src/optimization/first_order/lbfgs.rs 92.85% <0.00%> (-1.59%) ⬇️
src/math/num.rs 82.69% <0.00%> (+3.84%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Mec-iS
Copy link
Collaborator

Mec-iS commented Sep 26, 2022

please add the breaking change to the CHANGESLOG

@morenol
Copy link
Collaborator

morenol commented Oct 1, 2022

I think that having the Kernel trait give us more flexibility. More people could implement their own Kernel and use it without troubles. I think that what we could do for ease of use is to convert Kernels into an enum and implement Kernel for Kernels.

I tried that approach in #181

Let me know if that is enough for your use case @montanalow

On the other hand, it looks like the original test is failing. Can you check that in your branch?
I got:

thread 'model_selection::hyper_tuning::tests::svm_check' panicked at 'index out of bounds: the len is 0 but the index is 0', src/svm/svc.rs:724:28

Copy link
Collaborator

@Mec-iS Mec-iS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adapt these changes to the new code in development.

Kernel is now an object-safe trait. Making an enum of Kernels had been a problem for me because a match expression needs to return the same type. I tried different options but for sure there is something I am missing:

pun enum Kernels {
    Linear(LinearKernel),
    RBF(RBFKernel)
    ...
}

match kernel {
    Linear => LinearKernel::default(),  // this returns a LinearKernel type
    RBF => RBFkernel::default(),       // this returns a RBFKernel type
    ...
}

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.

4 participants