-
Notifications
You must be signed in to change notification settings - Fork 47
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 Mixture Model #137
Conversation
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.
Thank you for your contribution! The PR LGTM :)
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.
Lots of really minor comments. Overall this is looking great!
lib/scholar/cluster/gmm.ex
Outdated
expectation of the Gaussian assignment for each data point x and the M-step which updates the | ||
parameters to maximize the expectations found in E-step. While every iteration of the algorithm | ||
is guaranteed to improve the log-likelihood, the final result depends on the initial values of | ||
the parameters and the entire procedure should be repeated several times. |
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.
From reading without looking at the code beforehand:
Does this mean that we need to run the algorithm multiple times, or does the algorithm itself repeat this procedure?
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.
The algorithm itself repeats the procedure num_runs
amount of times, each time with different initial parameters. Similarly as it was already done in k-means implementation.
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 am not sure if I should change anything here. Thoughts?
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, the description is clear. @polvalente do you have any suggestions?
Typo fixed. Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Paulo Valente <[email protected]>
Co-authored-by: Paulo Valente <[email protected]>
@msluszniak @polvalente Thanks for the support and given feedback! I've addressed most of the suggestions provided; see the comments above (except for vectorization that I would leave for now). There are few more remarks that I would like to make; see some of the comments I've written. |
Are there any blockers for this PR? |
@msluszniak Don't think so. I just need to address few suggestions that you made. Could you have a look at the two reviews I opened? The |
Sure, but tbh I cannot see them 😅 |
@msluszniak They are probably collapsed among many others :) |
Are you sure that your comments are not in |
Sorry, they were in pending mode and I don't think I am able to submit them. Hence, I'll write them here. Line 292: doing {num_gaussians, num_features, num_features} = Nx.shape(covariances) inside |
I'm almost sure that there is no such function implemented in This line won't work in |
You can use |
I see, thanks. Here it is not an issue, just wanted to check that this is intended. |
lib/scholar/cluster/gmm.ex
Outdated
}, | ||
k < num_gaussians do | ||
diff = x - means[k] | ||
covariance = Nx.dot(responsibilities[[.., k]] * Nx.transpose(diff), diff) / nk[k] |
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.
covariance = Nx.dot(responsibilities[[.., k]] * Nx.transpose(diff), diff) / nk[k] | |
covariance = Nx.dot(responsibilities[[.., k]] * diff, [-2], diff / nk[k], [-2]) |
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.
This won't work as Nx.transpose
is only applied to diff
, not the entire product. Adding another axis to responsibilities[[.., k]]
solves the problem.
covariance = Nx.dot(diff * Nx.new_axis(responsibilities[[.., k]], 1), [0], diff / nk[k], [0])
@krstopro please verify @polvalente suggestions above and we can ship it :) |
💚 💙 💜 💛 ❤️ |
Added Gaussian Mixture Model implementation, similar to the one in scikit-learn Python module. Currently it supports only full covariances and uses k-means for the initialisation. Other covariance types (diagonal, tied, spherical) and initialisation methods (random, k-means++) can easily be added.