[MRG+2] DOC correct docstring for sample_gaussian #6957

kwohlfahrt · 2016-07-04T14:57:43Z

Reference Issue

None.

What does this implement/fix? Explain your changes.

Docstring for sklearn.mixture.sample_gaussian did not match function behaviour. Caused some trouble trying to implement a compatible version for a different distribution. Corrects this for future reference.

tguillemot · 2016-07-04T15:19:59Z

@kwohlfahrt Thanks.
Indeed there is a problem with the doc of the function, but as the GMM class is deprecated, those function will be remove at version 0.20.
I have forgotten to put a deprecation warning to say to use make_blob instead.
Can you add the deprecation warning with your solution ?

kwohlfahrt · 2016-07-04T20:20:32Z

Ah. It looks to me like the replacement does not allow for different covariances, is that correct?

tguillemot · 2016-07-05T07:50:38Z

Indeed you right. I thought that make_blob could have any kind of covariances.
In fact, what you should use is numpy.multivariate_normal which is exactly the sample_gaussian function.

tguillemot · 2016-07-05T14:15:18Z

LGTM.
Can you change your title to [MRG+1] ?
@agramfort Can you have a look ?

agramfort · 2016-07-05T20:07:05Z

sklearn/mixture/gmm.py

@@ -64,7 +64,8 @@ def log_multivariate_normal_density(X, means, covars, covariance_type='diag'):
 return log_multivariate_normal_density_dict[covariance_type](
 X, means, covars)

-
+@deprecated("The class function sample_gaussian is deprecated and "
+ "will be removed in 0.20. Use numpy.multivariate_normal instead.")


if you deprecate a function you need to remove it from everywhere. is this used somewhere? otherwsie the test suite will throw a lot of deprecation warnings

Looking at the logs, there is only one test which uses it, test_sample_gaussian.

If you do git grep sample_gaussian, you'll see that the class _GMM uses it.
So we need to create a _sample_gaussian function.

tguillemot · 2016-07-06T07:43:32Z

@agramfort Sorry I've forgot that.
What I suggest is to create a private function with the name _sample_gaussian and create another public function call sample_gaussian which calls it. We have done a similar thing with the GMM class.

It will solve any problems until we remove completely the old GMM classes.

agramfort · 2016-07-06T08:03:25Z

sounds like a plan

tguillemot · 2016-07-07T00:55:26Z

Looking at the logs, there is only one test which uses it, test_sample_gaussian

With git grep sample_gaussian, it appears that the _GMM class uses sample_gaussian in sample.
So we need to create a _sample_gaussian function.

kwohlfahrt · 2016-07-07T17:14:00Z

OK. Should the tests that test sample_gaussian directly be moved to the new function as well?

agramfort · 2016-07-07T17:26:21Z

Yes

On Thu, Jul 7, 2016 at 7:14 PM +0200, "Kai" [email protected] wrote:

OK. Should the tests that test sample_gaussian directly be moved to the new function as well?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

agramfort · 2016-07-07T19:32:52Z

sklearn/mixture/gmm.py

@@ -64,7 +64,8 @@ def log_multivariate_normal_density(X, means, covars, covariance_type='diag'):
 return log_multivariate_normal_density_dict[covariance_type](
 X, means, covars)

-
+@deprecated("The function sample_gaussian is deprecated and "
+ "will be removed in 0.20. Use numpy.multivariate_normal instead.")


I suppose it's numpy.random.multivariate_normal

Indeed it's numpy.random.multivariate_normal.

agramfort · 2016-07-07T19:35:23Z

CIs are not happy

tguillemot · 2016-07-07T22:00:40Z

You have three other things to change in test_gmm.py:

add the import from sklearn.mixture.gmm import _sample_gaussian
remove all the mixture. before sample_gaussian
change the comment l26 #Test sample generation from mixture.sample_gaussian... to ... gmm._sample_gaussian...
remove the import l59 from sklearn.mixture import sample_gaussian

tguillemot · 2016-07-12T04:02:23Z

I think the appveyor failure is unrelated but to be sure can you do git commit --amend; git push -f.

amueller · 2016-07-28T19:58:02Z

sklearn/mixture/gmm.py

@@ -64,7 +64,9 @@ def log_multivariate_normal_density(X, means, covars, covariance_type='diag'):
 return log_multivariate_normal_density_dict[covariance_type](
 X, means, covars)

-
+@deprecated("The function sample_gaussian is deprecated and "


Please add the version they were deprecated in to the message (was deprecated in 0.18)

amueller · 2016-07-28T19:58:53Z

LGTM apart from adding the deprecation version. This was not superseded by #7012, was it?

tguillemot · 2016-07-29T12:42:30Z

@amueller No as @kwohlfahrt was working on it I didn't want to remove his work.
@kwohlfahrt Can you fix the comments ?

Docstring did not match function behaviour. Caused some trouble trying to implement a compatible version for a different distribution.

This avoids triggering the deprecation warning. sample_gaussian is just a thin wrapper.

_sample_gaussian is not publicly exported in mixture, so use gmm module.

kwohlfahrt · 2016-10-04T09:30:08Z

Hi all, sorry I dropped this for so long. Rebased and fixed the tests, so if you still want to merge go ahead!

amueller · 2016-10-05T19:56:22Z

lgtm. @tguillemot ?

amueller · 2016-10-25T21:12:07Z

@tguillemot good to merge?

tguillemot · 2016-10-26T08:21:49Z

LGTM. Sorry for the delay.

TomDLT · 2016-10-26T14:27:35Z

Thanks @kwohlfahrt

* DOC correct docstring for sample_gaussian Docstring did not match function behaviour. Caused some trouble trying to implement a compatible version for a different distribution. * MAINT Use _sample_gaussian internally This avoids triggering the deprecation warning. sample_gaussian is just a thin wrapper. * Fix _sample_gaussian test path _sample_gaussian is not publicly exported in mixture, so use gmm module.

kwohlfahrt changed the title ~~[MRG] DOC correct docstring for sample_gaussian~~ [MRG+1] DOC correct docstring for sample_gaussian Jul 5, 2016

agramfort reviewed Jul 5, 2016
View reviewed changes

agramfort reviewed Jul 7, 2016
View reviewed changes

tguillemot mentioned this pull request Jul 16, 2016

[MRG+1] Deprecated GMM functions #7012

Merged

amueller reviewed Jul 28, 2016
View reviewed changes

amueller changed the title ~~[MRG+1] DOC correct docstring for sample_gaussian~~ [MRG+2] DOC correct docstring for sample_gaussian Jul 28, 2016

DOC correct docstring for sample_gaussian

b527f84

Docstring did not match function behaviour. Caused some trouble trying to implement a compatible version for a different distribution.

kwohlfahrt force-pushed the master branch from e5daebf to 6c1805e Compare October 3, 2016 14:29

Kai Wohlfahrt added 2 commits October 3, 2016 16:04

MAINT Use _sample_gaussian internally

23a049c

This avoids triggering the deprecation warning. sample_gaussian is just a thin wrapper.

Fix _sample_gaussian test path

7b84efd

_sample_gaussian is not publicly exported in mixture, so use gmm module.

kwohlfahrt force-pushed the master branch from 6c1805e to 7b84efd Compare October 3, 2016 15:22

TomDLT merged commit 379f54b into scikit-learn:master Oct 26, 2016

rth mentioned this pull request Oct 9, 2017

Function sample_gaussian returns nothing #9893

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+2] DOC correct docstring for sample_gaussian #6957

[MRG+2] DOC correct docstring for sample_gaussian #6957

kwohlfahrt commented Jul 4, 2016

tguillemot commented Jul 4, 2016

kwohlfahrt commented Jul 4, 2016

tguillemot commented Jul 5, 2016

tguillemot commented Jul 5, 2016

agramfort Jul 5, 2016

kwohlfahrt Jul 6, 2016

tguillemot Jul 7, 2016

tguillemot commented Jul 6, 2016

agramfort commented Jul 6, 2016 via email

tguillemot commented Jul 7, 2016

kwohlfahrt commented Jul 7, 2016

agramfort commented Jul 7, 2016

agramfort Jul 7, 2016

tguillemot Jul 7, 2016

agramfort commented Jul 7, 2016

tguillemot commented Jul 7, 2016

tguillemot commented Jul 12, 2016 •

edited

Loading

amueller Jul 28, 2016

amueller commented Jul 28, 2016 •

edited

Loading

tguillemot commented Jul 29, 2016

kwohlfahrt commented Oct 4, 2016

amueller commented Oct 5, 2016

amueller commented Oct 25, 2016

tguillemot commented Oct 26, 2016

TomDLT commented Oct 26, 2016

[MRG+2] DOC correct docstring for sample_gaussian #6957

[MRG+2] DOC correct docstring for sample_gaussian #6957

Conversation

kwohlfahrt commented Jul 4, 2016

Reference Issue

What does this implement/fix? Explain your changes.

tguillemot commented Jul 4, 2016

kwohlfahrt commented Jul 4, 2016

tguillemot commented Jul 5, 2016

tguillemot commented Jul 5, 2016

agramfort Jul 5, 2016

Choose a reason for hiding this comment

kwohlfahrt Jul 6, 2016

Choose a reason for hiding this comment

tguillemot Jul 7, 2016

Choose a reason for hiding this comment

tguillemot commented Jul 6, 2016

agramfort commented Jul 6, 2016 via email

tguillemot commented Jul 7, 2016

kwohlfahrt commented Jul 7, 2016

agramfort commented Jul 7, 2016

agramfort Jul 7, 2016

Choose a reason for hiding this comment

tguillemot Jul 7, 2016

Choose a reason for hiding this comment

agramfort commented Jul 7, 2016

tguillemot commented Jul 7, 2016

tguillemot commented Jul 12, 2016 • edited Loading

amueller Jul 28, 2016

Choose a reason for hiding this comment

amueller commented Jul 28, 2016 • edited Loading

tguillemot commented Jul 29, 2016

kwohlfahrt commented Oct 4, 2016

amueller commented Oct 5, 2016

amueller commented Oct 25, 2016

tguillemot commented Oct 26, 2016

TomDLT commented Oct 26, 2016

tguillemot commented Jul 12, 2016 •

edited

Loading

amueller commented Jul 28, 2016 •

edited

Loading