Skip to content

Use Box in SVM and remove lifetimes #228

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Nov 4, 2022

Conversation

morenol
Copy link
Collaborator

@morenol morenol commented Nov 3, 2022

Fixes #

  • Remove lifetimes in Kernel trait.
  • Use box instead of & for Kernel objects n SVM structs.

Checklist

  • My branch is up-to-date with development branch.
  • Everything works and tested on latest stable Rust.
  • Coverage and Linting have been applied

Current behaviour

New expected behaviour

Change logs

@morenol morenol marked this pull request as ready for review November 3, 2022 22:45
@morenol morenol requested a review from Mec-iS as a code owner November 3, 2022 22:45
@Mec-iS
Copy link
Collaborator

Mec-iS commented Nov 3, 2022

i like the idea of making the kernels name with static lifetime but we cannot use Box for the kernel as it ruins the API call. For an example of how kernels are used see the Jupyter notebooks:

let y_hat: Vec<f64> = SVC::fit(
    &x, &y,
    &SVCParameters::default()
        .with_c(1.0)
        .with_kernel(&Kernels::rbf()
            .with_gamma(0.7)),
     ) .and_then(
        |lr| lr.predict(&x)
) .unwrap(); 

how the kernels are used in tests is irrelevant. we don't want the end-users to allocate to the heap manually.
Kernels are intended to be used as references in parameters and it is made possible by them being object-safe (dyn...). As far as I understood, for object-safe trait using & or using box is the same as they are dynamically allocated at runtime, see https://www.reddit.com/r/rust/comments/kw8p5v/what_does_it_mean_to_be_an_objectsafe_trait/ or "Advanced Traits" in the Rust book.

@Mec-iS
Copy link
Collaborator

Mec-iS commented Nov 3, 2022

about implementing an enum for kernels, see #221

@morenol morenol force-pushed the lmm/lifetimes_kernel branch from 22d9f44 to 89d2bbb Compare November 4, 2022 15:21
@Mec-iS
Copy link
Collaborator

Mec-iS commented Nov 4, 2022

be careful. I agreed in using 'static for the strings used by kernels' names, but if you implement the whole Kernel as 'static itself it means that you are keeping in memory every kernel instantiated for the entire length of the program.

My idea was to make the string names static, then implement an enumerator to convert string->kernel object, see #221 there is a library that allows doing string to object instantiation but I am not sure that it is viable.

@Mec-iS
Copy link
Collaborator

Mec-iS commented Nov 4, 2022

@morenol Ok then, please keep the &dyn Kernel syntax for type declarations as it is the preferred one since Rust 2021.
The rest is OK.

Good one!

@Mec-iS Mec-iS merged commit 425c3c1 into smartcorelib:development Nov 4, 2022
@morenol morenol deleted the lmm/lifetimes_kernel branch November 4, 2022 22:38
morenol added a commit that referenced this pull request Nov 8, 2022
* Do not change external API
Authored-by: Luis Moreno <morenol@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants