Skip to content

[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

Merged

Conversation

tguillemot
Copy link
Contributor

The deprecation of VBGMM is not done correctly indeed when we call VBGMM we have two deprecation messages: one for VBGMM one for DPGMM.
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 ?

@agramfort
Copy link
Member

LGTM

@tguillemot tguillemot changed the title Correct Deprecation of DPGMM and VBGMM. [MRG+1] Correct Deprecation of DPGMM and VBGMM. Aug 1, 2016
@@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why that change?

Copy link
Contributor Author

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.

@agramfort
Copy link
Member

@amueller merge if you're happy.

+1 on my side

@tguillemot
Copy link
Contributor Author

@amueller @ogrisel I really need this to be merged before you review #6651.

@tguillemot
Copy link
Contributor Author

@ngoix If you have some time to review this, it's related to the Bayesian Gaussian Mixture.

@ngoix
Copy link
Contributor

ngoix commented Aug 5, 2016

This looks good to me!

@tguillemot tguillemot changed the title [MRG+1] Correct Deprecation of DPGMM and VBGMM. [MRG+2] Correct Deprecation of DPGMM and VBGMM. Aug 5, 2016
@tguillemot
Copy link
Contributor Author

@agramfort @ngoix Thanks.

@agramfort agramfort merged commit 05dfe6c into scikit-learn:master Aug 6, 2016
TomDLT pushed a commit to TomDLT/scikit-learn that referenced this pull request Oct 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants