{"action":"created","issue":{"url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/4429","labels_url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/4429/labels{/name}","comments_url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/4429/comments","events_url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/4429/events","html_url":"https://github.com/scikit-learn/scikit-learn/issues/4429","id":63289171,"number":4429,"title":"incorrect estimated means lead to non positive definite covariance in GMM","user":{"login":"xuewei4d","id":1180956,"avatar_url":"https://avatars.githubusercontent.com/u/1180956?v=3","gravatar_id":"","url":"https://api.github.com/users/xuewei4d","html_url":"https://github.com/xuewei4d","followers_url":"https://api.github.com/users/xuewei4d/followers","following_url":"https://api.github.com/users/xuewei4d/following{/other_user}","gists_url":"https://api.github.com/users/xuewei4d/gists{/gist_id}","starred_url":"https://api.github.com/users/xuewei4d/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/xuewei4d/subscriptions","organizations_url":"https://api.github.com/users/xuewei4d/orgs","repos_url":"https://api.github.com/users/xuewei4d/repos","events_url":"https://api.github.com/users/xuewei4d/events{/privacy}","received_events_url":"https://api.github.com/users/xuewei4d/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2015-03-20T19:35:34Z","updated_at":"2015-03-20T20:11:50Z","closed_at":null,"body":"I dig into #3945 and #2640. PR #3945 claims\r\n\r\n > Due to round off errors, the formula used to compute the weighted covariance matrix may lead to matrices with large negative eigenvalues.\r\n\r\nActually, the reason why the sample code given by @AlexisMignon in #2640 raises an exception  is that the simplified formula C = (\\sum_i w_i x_i x_i') - \\mu \\mu' does not apply when \\mu is not the mean of X.\r\n\r\nThe gmm created in this line \r\n``` Python\r\ngmm = mixt.GMM(2, params=\"wc\", covariance_type=\"full\", min_covar=1e-3)\r\n```\r\ndoes not estimate ```mean``` in M-step. The ```gmm.means_``` is initialized by k-means, and does not change in EM. When the code in m_step before #3945 merged is executed, in one of component the mean is initialized to ```[[ 3.42118979  3.53824541]]``` by kmeans.  However, the real mean is ```[0.02670257  0.02945094]``` before the exception is raised. Using C = (\\sum_i w_i x_i x_i') - \\mu \\mu',  of course, the estimated covariance is not positive definite. If\r\n``` Python\r\ngmm = mixt.GMM(2, params=\"wmc\", covariance_type=\"full\", min_covar=1e-3)\r\n```\r\nWe don't have the exception at all.\r\n\r\nIn #2640, without the snippet resulting the error, I also cannot figure out why the covariance is not positive definite. Anyway, I think the method ```log_multivariate_normal_density``` in gmm.py could be called by methods in other modules, so it is better to check the covariance first before Cholesky decomposition. \r\n\r\nBesides, when using parameter ```params='wc'``` to initialize a gmm, we ~~always~~ have non-positive-definite covariance problems, with ```full``` and ```tied``` type covariance. You could try it with any random generated data. I think it is strange to build GMM without estimating means. It would be better to deprecate ```params``` initialization other than ```wmc```.\r\n\r\nFinally, the original code using equation C = (\\sum_i w_i x_i x_i') - \\mu \\mu' before #3945 merged, I think, it is OK as long as we could guarantee \\mu is the mean of X. But I found the code in Matlab use the way as #3945 does. I cannot see other reasons to use that way except preventing round-off errors, which I really doubt.\r\n\r\nPing @amueller \r\n\r\n\r\n\r\n\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/comments/84132768","html_url":"https://github.com/scikit-learn/scikit-learn/issues/4429#issuecomment-84132768","issue_url":"https://api.github.com/repos/scikit-learn/scikit-learn/issues/4429","id":84132768,"user":{"login":"amueller","id":449558,"avatar_url":"https://avatars.githubusercontent.com/u/449558?v=3","gravatar_id":"","url":"https://api.github.com/users/amueller","html_url":"https://github.com/amueller","followers_url":"https://api.github.com/users/amueller/followers","following_url":"https://api.github.com/users/amueller/following{/other_user}","gists_url":"https://api.github.com/users/amueller/gists{/gist_id}","starred_url":"https://api.github.com/users/amueller/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amueller/subscriptions","organizations_url":"https://api.github.com/users/amueller/orgs","repos_url":"https://api.github.com/users/amueller/repos","events_url":"https://api.github.com/users/amueller/events{/privacy}","received_events_url":"https://api.github.com/users/amueller/received_events","type":"User","site_admin":false},"created_at":"2015-03-20T20:11:50Z","updated_at":"2015-03-20T20:11:50Z","body":"I see. I agree it would be nice to catch the error. I thought it was more informative.\r\nI think the intention behind the \"duplication\" between ``init_params`` and ``params`` is that you might want to a) give a custom initialization, but want to keep on training (``init_params`` allows that), or want to set some fixed parameters and not train them at all (``params`` allows that).\r\nI think we want to deprecate ``params`` because estimating some parameters and not others can lead to trouble.\r\n\r\nFor ``init_params`` it is not something we usually support in scikit-learn, but I can see how it could be useful.\r\nA different way to do this would be to actually pass the initialization as parameters, which is what we do in different places, like ``means_init=some_array, covars_init=some_array`` etc."}}