[MRG+2] Modification of GaussianMixture class.#7123
[MRG+2] Modification of GaussianMixture class.#7123agramfort merged 5 commits intoscikit-learn:masterfrom
Conversation
The purpose here is to prepare the integration of BayesianGaussianMixture.
| if do_init: | ||
| self._initialize_parameters(X) | ||
| current_log_likelihood, resp = self._e_step(X) | ||
| self.lower_bound_ = np.infty |
There was a problem hiding this comment.
shouldn't you document the new attribute?
There was a problem hiding this comment.
Indeed, sorry for that mistake.
There was a problem hiding this comment.
I didn't know np.infty also returns np.inf... Interesting...
There was a problem hiding this comment.
Shouldn't it be self.lower_bound_ = -np.infty ?
There was a problem hiding this comment.
It has been merged but I re-ask the question: shouldn't it be -np.infty ?
|
that's it for me |
| X : array-like, shape (n_samples, n_features) | ||
| """ | ||
| n_samples = X.shape[0] | ||
| n_samples, _ = X.shape |
There was a problem hiding this comment.
Sorry if this was suggested before...
There was a problem hiding this comment.
It just I prefer like this and it also check that the shape of X is a tuple.
Sorry for these useless little modifications.
There was a problem hiding this comment.
it also checks implicitly that X.ndim == 2. It do the same in my code.
| n_iter_ : int | ||
| Number of step used by the best fit of EM to reach the convergence. | ||
|
|
||
| lower_bound_ : float |
There was a problem hiding this comment.
lower_bound_ -> best_log_likelihood_ ?
There was a problem hiding this comment.
In fact for GMM this is the best log likelihood but not for VBGMM which is lower bound.
I've chosen lower_bound_ because it was the most understandable.
There was a problem hiding this comment.
|
@ngoix This PR is related to Bayesian Gaussian Mixture, if you have some time to review it. |
|
one more +1 and we're good here... |
sklearn/mixture/base.py
Outdated
|
|
||
| for n_iter in range(self.max_iter): | ||
| prev_log_likelihood = current_log_likelihood | ||
| prev_log_likelihood = self.lower_bound_ |
There was a problem hiding this comment.
prev_log_likelihood -> prev_lower_bound ?
|
By testing the code, I realized that there is a problem which was here before this PR: |
|
|
||
| def _compute_lower_bound(self, _, log_prob_norm): | ||
| return log_prob_norm | ||
|
|
There was a problem hiding this comment.
In _set_parameters below, compute self.covariances_ from self.precisions_.
|
That's all from me! |
|
@ngoix Indeed you're right. I've missed that. Thanks. |
|
@agramfort merge ? |
The PR is to simplify the integration of the
BayesianGaussianMixtureclass #6651.I've simplify the
GaussianMixtureclass by integrating a function which computes the determinant of the cholesky decomposition of the precision matrix (which will be usefull forBayesianGaussianMixture).I've also corrected a bug during the EM process: normally, the lower bound is computed after the M-step not after the E-step. It's not a problem for GMM but that creates some problem for
BayesianGaussianMixture.@agramfort @ogrisel @amueller Can you have a look ?