Fix GaussianMixture UnboundLocalError #20030
Conversation
|
@jjerphan could you please take a look? |
I am wondering whether you think it's fine to specify the initialization parameters directly? They are actually calculated using the part of the code in |
There was a problem hiding this comment.
Here are a few suggestions @tliu68.
Actually your contribution fixes more than just the best parameters in the case of divergence for mixture.GaussianMixture: a similar problem could also occur for mixture.BayesianGaussianMixture, where their best parameters (weight_concentration_, mean_precision_, means_, degrees_of_freedom_, covariances_, precisions_cholesky_) not being set.
Regarding your previous comment, in-line definition of parameters is fine as long as you comment like you did. 👍
| def test_gaussian_mixture_setting_best_params(): | ||
| # test that best_params is set appropriately | ||
| # even if convergence was not reached | ||
| # also a regression test for issue #18216 |
There was a problem hiding this comment.
Ideally, we would like to create another test for BayesianMixture, but this might be hard (we need to find a set of initial parameters which would cause it to diverge). You can try to see if the parameters you've in-lined cause this problem for BayesianMixture. If it doesn't, it's fine not going down the rabbit hole to find ones that do.
| def test_gaussian_mixture_setting_best_params(): | |
| # test that best_params is set appropriately | |
| # even if convergence was not reached | |
| # also a regression test for issue #18216 | |
| def test_gaussian_mixture_params_setting(): | |
| """`GaussianMixture`'s best_parameters, `n_iter_` and `lower_bound_` | |
| must be set appropriately in the case of divergence. | |
| Non-regression test for: https://github.com/scikit-learn/scikit-learn/issues/18216 | |
| """ |
There was a problem hiding this comment.
@tliu68 Thanks for the fix.
Could you also replace the 2 orccurrances of np.infty by np.inf in mixture/_base.py?
|
@tliu68 Could you also put an entry in whats_new? |
…xture Model PR#11101, focused on allowing sample-based and k-means++ based GMM initialization.. Also made the enhancements compatible with PRs scikit-learn#17937 and scikit-learn#20030.
…sian Mixture Model PR#11101, focused on allowing sample-based and k-means++ based GMM initialization.. Also made the enhancements compatible with PRs scikit-learn#17937 and scikit-learn#20030.
Reference Issues/PRs
Fixes issue #18216
What does this implement/fix? Explain your changes.
Updates the condition on
lower_boundto ensure thatbest_paramsis set even if convergence was not reached.Any other comments?