-
-
Notifications
You must be signed in to change notification settings - Fork 238
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
Gaussian Naive Bayes (GaussianNB) #51
Conversation
will look into this in the weekend :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it makes more sense to structure it into ModelParameter
-> FittedModel
, than ProbablyFittedModel
-> FittedModel
. Added a comment how the API would look like, what do you think?
77344e2
to
51ad204
Compare
Could you take a look at https://github.com/rust-ml/linfa/blob/fdf241127fe9058ae98c67a603a2bbb8eccc561c/CONTRIBUTE.md, I'm currently writing a small how-to-contribute document for the upcoming interface changes. If you find something unclear, please let me know so I can update the document. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the review with regards to merging #55
If you have any questions how to implement the new traits, please let me know!
@bytesnake Thank you for the detailed comments, will try to make the changes by next week. |
0f4bfbc
to
9ed89ce
Compare
@bytesnake For incrementally learning the algorithm needs the definite set of classes supplied by the user, currently I supply them as an argument in the |
I think you can use the fn labels(&self) -> Vec<T::Elem> returning a vector of the unique classes. For this you have to restrict your targets like this: impl<F: Float, R: Records<Elem = F>, T: Labels<Elem = usize>> IncrementalFit<R, T> for GaussianNbParams<F> {
} in the future the trait could also be extended to accept any countable type: impl<F: Float, L: Label, R: Records<Elem = F>, T: Label<Elem = L>> IncrementalFit<R, T> for GaussianNbParams<F, L> {
} |
oh and you could estimate the priors from the samples as well, there is the Dataset::frequencies_with_mask function. It was introduced for decision trees, where you have to mask the data at each split point. You could either pass a |
d09514d
to
19b0747
Compare
@bytesnake I have started using the But the issue with incremental learning in naive-bayes is that we need to know the complete list of labels while training on batches of data, the first batch of data that is fed to the model will almost never have encountered all the labels, that is the reason why I have an extra argument in the function signature. Also I see |
@bytesnake Currently I calculate priors along with mean and variance for each class, which requires masked indexing for each class. |
@VasanthakumarV I think that an incremental model should collect its label incrementally as well. Would it be possible to form the union of previous labels with new encounter ones and update the model correspondingly. Otherwise you could also pass the labels as a hyper-parameter and return an error when a different label is encountered. |
the API around dataset is very sparse right now, any update is welcome and a function which can select samples based on the class should definitely be in there somewhere 👍 It would be even more useful if you could somehow produce a view on the sub-selected samples, but I don't think that is possible right now. |
e0f2526
to
1b7fbf6
Compare
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
==========================================
+ Coverage 49.07% 49.33% +0.25%
==========================================
Files 55 57 +2
Lines 3629 3762 +133
==========================================
+ Hits 1781 1856 +75
- Misses 1848 1906 +58
Continue to review full report at Codecov.
|
1b7fbf6
to
5c04b81
Compare
@bytesnake I went with the first approach from a usability standpoint. To implement the incremental learning API, I had to change IncrementalFit API - link. The big problem right now is IncrementalFit looks different from Fit trait. I wasn't able to make Option and Result work together in the old incremental trait, I am still thinking about ways to achieve what I want with a trait that fits nicely with everything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for all the work, will merge after adressing these small nitpicks 👍
c4c0d77
to
f94fe21
Compare
ready to merge? |
I really don't like the changes I made to IncrementalFIt trait, especially hardcoding Result on the output. I might take another day to make it better. |
@bytesnake What do you think about the following trait for IncrementalFit?, two associated types to capture the asymmetry. pub trait IncrementalFit<'a, R: Records, T: Targets> {
type Input: 'a;
type Output: 'a;
fn fit_with(&self, model: Self::Input, dataset: &'a Dataset<R, T>) -> Self::Output;
}
// Implementation
impl<A, L> IncrementalFit<'_, ArrayView2<'_, A>, L> for GaussianNbParams
where
A: Float,
L: Labels<Elem = usize>,
{
type Input = Option<GaussianNb<A>>;
type Output = Result<Option<GaussianNb<A>>>;
fn fit_with(&self, model_in: Self::Input, dataset: &Dataset<ArrayView2<A>, L>) -> Self::Output {
...
}
...
} The problem here is the flexibility, no rule is enforced, but the user has the freedom to either return an Option or an Option wrapped in a result should there be a need. |
didn't see this yesterday, this definitely gives you more flexibility. I had another thing in my mind, which I pushed here VasanthakumarV#1 |
I'm back from christmas and after the break I think your idea is good, especially in combination with |
871c0f8
to
c10a51e
Compare
@bytesnake do you want to rename the associated type's names from |
c10a51e
to
2f324f9
Compare
@VasanthakumarV no |
2f324f9
to
bbd7b95
Compare
I made them |
Port of GaussianNB