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+1] Integration of GSoC2015 Gaussian Mixture (first step) #6666

Merged
merged 2 commits into from Apr 21, 2016

Conversation

Projects
None yet
7 participants
@tguillemot
Contributor

tguillemot commented Apr 15, 2016

@ogrisel @agramfort @TomDLT I've created a new PR to solve the problem with travis of the #6407 .
Sorry for the noise.

http://www.mixmod.org
https://cran.r-project.org/web/views/Cluster.html (bgmm for instance)

@ogrisel Mixmod and bgmm don't divide the tolerance by n_samples.

for _ in range(100):
prev_log_likelihood = current_log_likelihood
current_log_likelihood = gmm.fit(X).score(X)
assert_greater(current_log_likelihood, prev_log_likelihood)

This comment has been minimized.

@ogrisel

ogrisel Apr 15, 2016

Member

I think assert_greater_equal is safer here. There is no guarantee for strict monotonicity.

This comment has been minimized.

@ogrisel

ogrisel Apr 15, 2016

Member

Also this test should catch ConvergenceWarning explicitly.

X = rand_data.X[cov_type]
gmm = GaussianMixture(n_components=n_components,
covariance_type=cov_type, reg_covar=0,
warm_start=True, max_iter=20, random_state=rng)

This comment has been minimized.

@ogrisel

ogrisel Apr 15, 2016

Member

Please test with one iteration at time: max_iter=1 for a stricter check of the mathematical property we are interested in.

assert_equal(gmm_score_samples.shape[0], rand_data.n_samples)
def test_monotonic_likelihood():

This comment has been minimized.

@ogrisel

ogrisel Apr 15, 2016

Member

Please add an inline comment to explain this test: we check that each step of classical EM without regularization should monotonically improve the training set likelihood.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 15, 2016

When launching the tests I get a numerical underflow here:

sklearn.mixture.tests.test_gaussian_mixture.test_gaussian_mixture_estimate_log_prob_resp ... /home/ogrisel/code/scikit-learn/sklearn/utils/extmath.py:411: RuntimeWarning: underflow encountered in exp
  out = np.log(np.sum(np.exp(arr - vmax), axis=0))

this could easily be resolved with scipy.misc.logsumexp (I think).

@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 15, 2016

There is another logsumexp issue here:

sklearn.mixture.tests.test_gaussian_mixture.test_gaussian_mixture_estimate_log_prob_resp ... /home/ogrisel/code/scikit-learn/sklearn/utils/extmath.py:411: RuntimeWarning: underflow encountered in exp
  out = np.log(np.sum(np.exp(arr - vmax), axis=0))
@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 15, 2016

This underflow might be more tricky but it would be great if you could have a look:

/home/ogrisel/code/scikit-learn/sklearn/mixture/tests/test_gaussian_mixture.py:448: RuntimeWarning: underflow encountered in exp
  resp = np.exp(log_resp)
@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 15, 2016

Please also make sure to filter the DeprecationWarnings for the old GMM class in the tests.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 15, 2016

There are also a couple of ConvergenceWarning that we should catch when they are expected (or fix if they are not expected):

sklearn.mixture.tests.test_gaussian_mixture.test_gaussian_mixture_aic_bic ... /home/ogrisel/code/scikit-learn/sklearn/mixture/base.py:218: ConvergenceWarning: Initialization 1 did not converged. Try different init parameters, or increase n_init, tol or check for degenerate data.
  % (init + 1), ConvergenceWarning)
ok
sklearn.mixture.tests.test_gaussian_mixture.test_warm_start ... /home/ogrisel/code/scikit-learn/sklearn/mixture/base.py:218: ConvergenceWarning: Initialization 1 did not converged. Try different init parameters, or increase n_init, tol or check for degenerate data.
  % (init + 1), ConvergenceWarning)
prev_log_likelihood = current_log_likelihood
current_log_likelihood = gmm.fit(X).score(X)
assert_greater_equal(current_log_likelihood, prev_log_likelihood)

This comment has been minimized.

@ogrisel

ogrisel Apr 15, 2016

Member

Maybe you could also add a check that gmm.n_iter_ is also incremented by 1 at each iteration.

This comment has been minimized.

@tguillemot

tguillemot Apr 18, 2016

Contributor

I'm not agree with for these one.
For me warm_start is a way to fit another model which is close to the previous fitted model.
Consequently, n_iter_ corresponds to the number of iterations use to fit the new model.
I can change the way it's implemented if you prefer.

assert_greater_equal(current_log_likelihood, prev_log_likelihood)
if gmm.converged_:
break

This comment has been minimized.

@ogrisel

ogrisel Apr 15, 2016

Member

Maybe also add:

assert_greater_equal(gmm.n_iter_, 10)

at the end of the loop.

@tguillemot

This comment has been minimized.

Contributor

tguillemot commented Apr 18, 2016

this could easily be resolved with scipy.misc.logsumexp (I think).

In fact, the problem is the same with scipy.misc.logsumexp.
I have change the test to solve this problem.

There is still another underflow in the regularisation test but this one is normal : I've filter it.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 18, 2016

Instead of using a catch-all @ignore_warnings it would be great to selectively filter DeprecationWarnings and add an inline comment to state that those are tests for the deprecated GMM class.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 18, 2016

This is also a good opportunity to review all those tests and check that there is an equivalent test for the GaussianMixture class when it makes sense.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 18, 2016

We still get the convergence warning by running most of the examples. I think the default value for tol is too strict. I think we should probably increase it to make it such that the GaussianMixture examples do not raise any convergence warning with the default convergence criterion.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 18, 2016

I would set the default tol to tol=1e-3 (don't forget to change the docstring of the GaussianMixture class accordingly) while also setting tol=1e-7 as non-default constructor argument only in the test_warm_start to check for stricter convergence.

@tguillemot

This comment has been minimized.

Contributor

tguillemot commented Apr 19, 2016

Ok I've added some tests to check the attribute values and that multiple init are giving better or equal results.

raise ValueError("The algorithm has diverged because of too "
"few samples per components. "
"Try to decrease the number of components, or "
"increase reg_covar.")

This comment has been minimized.

@ogrisel

ogrisel Apr 19, 2016

Member

It would be better to add make the error message more informative by including the current values for n_components and reg_covar.

This comment has been minimized.

@tguillemot

tguillemot Apr 20, 2016

Contributor

I write it here what we discuss earlier to push a trace of your talk on github.
I prefer to not change that function because I should add reg_covar as parameter only for the warning message.

"diag": _estimate_gaussian_covariance_diag,
"spherical": _estimate_gaussian_covariance_spherical}
nk = resp.sum(axis=0) + 10 * np.finfo(float).eps

This comment has been minimized.

@ogrisel

ogrisel Apr 19, 2016

Member

It's better to use np.finfo(resp.dtype).eps to make the code adaptive to a change of precision in dtype.

covariance_type='badcovariance_type')
# This function tests the deprecated old GMM class
with warnings.catch_warnings():
warnings.simplefilter("ignore", DeprecationWarning)

This comment has been minimized.

@ogrisel

ogrisel Apr 19, 2016

Member

Instead of repeating a with catch blocks in each test, it would be great to enrich the @ignore_warning function decorator to accept an additional categogy=DeprecationWarning argument to pass it down to the warnings.simplefilter call in _IgnoreWarnings.__enter__.

This comment has been minimized.

@ogrisel

ogrisel Apr 19, 2016

Member

Sorry for the back and forth review ;)

This comment has been minimized.

@tguillemot

tguillemot Apr 20, 2016

Contributor

Ok I've changed it in that PR but maybe it could be interested to do another PR with that.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 19, 2016

@tguillemot this was my last batch of comments for this PR. +1 for merge once they are addressed ;)

if callable(obj):
return _ignore_warnings(obj)
else:
elif category is None:
return _IgnoreWarnings()

This comment has been minimized.

@ogrisel

ogrisel Apr 20, 2016

Member

What I had in mind was to instead extend _IgnoreWarnings to accept a cagegory keyword that it would itself pass to its own underlying call to warnings.simplefilter in its __enter__ method.

@@ -331,11 +340,7 @@ class _IgnoreWarnings(object):

This comment has been minimized.

@ogrisel

ogrisel Apr 20, 2016

Member

The above docstring will have to be changed to reflect the fact that it can now accept a category kwarg in its constructor.

@tguillemot

This comment has been minimized.

Contributor

tguillemot commented Apr 21, 2016

Ok @ogrisel , it took a long time but I've modified the ignore_warning as you wanted.
I've added some tests to be sure it's working as expected.

"""Improved and simplified Python warnings context manager
This class allows to ignore the warnings raise by a function.
Copied from Python 2.7.5 and modified as required.

This comment has been minimized.

@ogrisel

ogrisel Apr 21, 2016

Member

This is no longer a straigth copy from Python 2.7.5. This part of the docstring needs to be updated.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 21, 2016

LGMT, +1 for merge once the docstring of _IgnoreWarning has been updated and if the tests pass on travis + appveyor. Please also squash the commits and add an entry in whats_new.rst. Please document the deprecation in the relevant section for 0.18.

@agramfort @TomDLT do you like the warnings change?

Integration of the new GaussianMixture class.
Depreciation of the GMM class.

Modification of the GaussianMixture class.
  Some functions from the original GSoC code have been removed, renamed or simplified.
  Some new functions have been introduced (as the 'check_parameters' function).
  Some parameters names have been changed :
   - covars_ -> covariances_ : to be coherent with sklearn/covariances
  Addition of the parameter 'warm_start' allowing to fit data by using the previous computation.

The old examples have been modified to replace the deprecated GMM class by the new GaussianMixture class.
Every exemple use the eigenvectors norm to solve the scale ellipse problem (Issues 6548).

Correction of all commentaries from the PR
- Rename MixtureBase -> BaseMixture
- Remove n_features_
- Fix some problems
- Add some tests

Correction of the bic/aic test.

Fix the test_check_means and test_check_covariances.

Remove all references to the deprecated GMM class.

Remove initialized_.
Add and correct docstring.

Correct the order of random_state.

Fix small typo.

Some fix in prevision of the integration of the new BayesianGaussianMixture class.

Modification in preparation of the integration of the BayesianGaussianMixture class.
Add 'best_n_iter' attribute.
Fix some bugs and tests.

Change the parameter order in the documentation.

Change best_n_iter_ name to n_iter_.

Fix of the warm_start problem.
Fix the divergence error message.
Correction of the random state init in the test file.
Fix the testing problems.

Update and add comments into the monotonic test.
@TomDLT

This comment has been minimized.

Member

TomDLT commented Apr 21, 2016

@TomDLT do you like the warning change?

Yep, looks fine to me.

@tguillemot tguillemot changed the title from [MRG] Integration of GSoC2015 Gaussian Mixture (first step) to [MRG+1] Integration of GSoC2015 Gaussian Mixture (first step) Apr 21, 2016

@ogrisel ogrisel merged commit 6bc346b into scikit-learn:master Apr 21, 2016

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.07%) to 94.106%
Details
@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 21, 2016

Thank you very much @tguillemot and @xuewei4d!

@GaelVaroquaux

This comment has been minimized.

Member

GaelVaroquaux commented Apr 21, 2016

@ogrisel

This comment has been minimized.

Member

ogrisel commented Apr 21, 2016

🍻

1 similar comment
@raghavrv

This comment has been minimized.

Member

raghavrv commented Apr 21, 2016

🍻

@xuewei4d

This comment has been minimized.

Contributor

xuewei4d commented Apr 22, 2016

🍻!

And sorry for not merging code. I am busy with writing papers.

@agramfort

This comment has been minimized.

Member

agramfort commented Apr 22, 2016

🍻 ** 2

@tguillemot

This comment has been minimized.

Contributor

tguillemot commented Apr 22, 2016

Youpi !!!
🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment