-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[MRG+2] Correct Deprecation of DPGMM and VBGMM. #7124
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
[MRG+2] Correct Deprecation of DPGMM and VBGMM. #7124
Conversation
LGTM |
@@ -309,7 +309,7 @@ def test_train_1d(self, params='wmc'): | |||
with ignore_warnings(category=DeprecationWarning): | |||
g.fit(X) | |||
trainll = g.score(X) | |||
if isinstance(g, mixture.DPGMM): |
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.
why that change?
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.
DPGMM and VBGMM are both deprecated and as VBGMM inherit from DPGMM you have two warnings messages when you call VBGMM.
I've corrected that to make sure the deprecation warning is meaningfull and moreover to be sure to pass the test once I will modify the deprecation message to refer to the new BayesianGaussianMixture class.
@amueller merge if you're happy. +1 on my side |
@ngoix If you have some time to review this, it's related to the Bayesian Gaussian Mixture. |
This looks good to me! |
@agramfort @ngoix Thanks. |
The deprecation of
VBGMM
is not done correctly indeed when we callVBGMM
we have two deprecation messages: one forVBGMM
one forDPGMM
.Consequently I have modify the code (as we have done before for the
GMM
class) to be sure that the right deprecation warning is launched.@agramfort @ogrisel @amueller Can you have a look please ?