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

Iter fold #77

Merged
merged 6 commits into from
Feb 1, 2021
Merged

Iter fold #77

merged 6 commits into from
Feb 1, 2021

Conversation

Sauro98
Copy link
Member

@Sauro98 Sauro98 commented Jan 27, 2021

This PR introduces the iter_fold method for k_folding. The main points are:

Linfa/src

  • Modified Fit trait to remove training set lifetime
  • Implemented iter_folding without cloning

Linfa-kernel

  • Introduced new trait Inner to represent an inner matrix. Implementors of this trait are: Array2, ArrayView2, CsMat, CsMatView
  • enum KernelInner now takes two Inner as parameters, one for the dense implementation and one for the sparse implementation
  • Renamed Kernel to KernelBase, which now takes the two Inner types for the KernelInner enum
  • Added type Kernel<'a, F> which is a kernel that keeps the data in an ArrayView2 and has an owned inner matrix (the most common case)
  • Added type KernelOwned<F> for which the data is owned too
  • Added type KernelView<'a, F> which has a view on both its data and inner matrix
  • Implemented functions view() and to_owned() to switch between the various types

This was done to be able to have both a kernel that borrows data with a lifetime and one that owns it with just one struct

Linfa-svm

  • Modified implementation so that now Svm owns the kernel data (via a KernelOwned attribute)
  • Adjusted to new Fit definition and kernel types
  • Added tests for iter_fold both for regression and classification to show that it works

Other sub crates

  • Adjuster for new Fit definition and kernel types

This doesn't address the points on #70 about the Dynamic Kernel type and Svm that only keeps its support vectors because I wanted to have a solid and approved basis before doing any additional work.

removed fit dataset lifetime, finished fold_fit
tentative trait impl for kernel
adjusted svm for iter_fold
@codecov-io
Copy link

Codecov Report

Merging #77 (a58943f) into master (923b82f) will decrease coverage by 0.46%.
The diff coverage is 38.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   54.49%   54.02%   -0.47%     
==========================================
  Files          62       63       +1     
  Lines        4318     4583     +265     
==========================================
+ Hits         2353     2476     +123     
- Misses       1965     2107     +142     
Impacted Files Coverage Δ
linfa-elasticnet/src/algorithm.rs 82.63% <ø> (ø)
linfa-linear/src/ols.rs 67.96% <ø> (ø)
linfa-reduction/src/diffusion_map/algorithms.rs 0.00% <0.00%> (ø)
linfa-kernel/src/inner.rs 19.11% <19.11%> (ø)
src/dataset/impl_dataset.rs 36.02% <26.31%> (-1.82%) ⬇️
linfa-svm/src/regression.rs 45.71% <26.66%> (+1.11%) ⬆️
linfa-svm/src/classification.rs 51.98% <28.00%> (-10.44%) ⬇️
linfa-svm/src/solver_smo.rs 36.29% <30.58%> (+0.98%) ⬆️
linfa-svm/src/permutable_kernel.rs 46.34% <31.57%> (-10.81%) ⬇️
linfa-kernel/src/lib.rs 71.78% <38.46%> (-1.97%) ⬇️
... and 17 more

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 923b82f...a58943f. Read the comment docs.

linfa-svm/src/lib.rs Outdated Show resolved Hide resolved
@bytesnake
Copy link
Member

bytesnake commented Jan 28, 2021

thanks! I will look into this in the next days :)

Copy link
Member

@bytesnake bytesnake left a comment

Choose a reason for hiding this comment

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

okay I added some comments on the PR. This is really awesome work 👍
I think it would be better to separate Kernel from Dataset in order to resolve the ambiguities between Kernel and OwnedKernel. I added some comments how this can be realized.

After merging this, I will investigate how to simplify the API with Borrow

linfa-kernel/src/lib.rs Show resolved Hide resolved
linfa-kernel/src/lib.rs Show resolved Hide resolved
linfa-svm/src/lib.rs Outdated Show resolved Hide resolved
linfa-svm/src/solver_smo.rs Show resolved Hide resolved
src/dataset/impl_dataset.rs Show resolved Hide resolved
linfa-svm/src/lib.rs Outdated Show resolved Hide resolved
@bytesnake
Copy link
Member

you still have to remove the commented parameters, but for my part we can merge this

@Sauro98
Copy link
Member Author

Sauro98 commented Feb 1, 2021

my bad, I forgot to remove them 👍🏻

@bytesnake bytesnake merged commit b0af805 into rust-ml:master Feb 1, 2021
@Sauro98 Sauro98 deleted the iter_fold branch February 1, 2021 21:10
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.

3 participants