Add GaussianMixtureModel in linfa-clustering #56
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bytesnake
reviewed
Nov 8, 2020
There was a problem hiding this comment.
Thanks for the work, I added some small comments.
- we should move away from implicit error handling with panics, I know that we do panic in a lot of places at the moment, but returning an error is much more flexible
GmmHyperParams::fitshould implicitly build the parameter set- can you add at least a second test for the algorithm?
otherwise the code is really clean and easy to read 👍 you have to rebase the PR to the latest commit with #55 merged
bytesnake
reviewed
Nov 8, 2020
bytesnake
reviewed
Nov 8, 2020
bytesnake
reviewed
Nov 8, 2020
bytesnake
reviewed
Nov 8, 2020
bytesnake
reviewed
Nov 8, 2020
relf
force-pushed
the
ft_gaussian_mixture
branch
from
84220f7
to
1f4a1d7
Compare
relf
force-pushed
the
ft_gaussian_mixture
branch
from
1f4a1d7
to
0e4cd84
Compare
bytesnake
approved these changes
Nov 13, 2020
|
Nope, it is ok for me as well. |
VasanthakumarV
pushed a commit
to VasanthakumarV/linfa
that referenced
this pull request
Dec 8, 2020
* Add GaussianMixtureModel clustering algorithm (rebase) * Add ndarray-linalg dependency * Add test-openblas-system feature * Fix from review * Return errors instead of panicking * Fix error management * Revert unwanted change * Add covariance, precisions getters and add another test * Add with_rng(), remove params_with_rng() * Use Lapack + Scalar bounds to get cholesky working * Remove GmmHyperParamsBuilder, add hyperparams invalid value tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a port of the Gaussian Mixture Model.
Some limitations wrt to the original:
I am fairly new to Rust programming, so feedback is welcome.