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

GMM -> GaussianMixture #175

Closed
chanansh opened this Issue Feb 12, 2017 · 24 comments

Comments

Projects
None yet
7 participants
@chanansh

chanansh commented Feb 12, 2017

In sklearn GMM was replaced by GaussianMixture.
See https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/mixture/gmm.py:

class GMM(_GMMBase):
"""
Legacy Gaussian Mixture Model
.. deprecated:: 0.18
This class will be removed in 0.20.
Use :class:sklearn.mixture.GaussianMixture instead.
"""

However, hmmlearn still uses the old version.
A pull request is needed to upgrade hmmlearn to work with the newer API.

@superbobry

This comment has been minimized.

Member

superbobry commented Feb 12, 2017

This is true. The master version has a re-implementation of GMMHMM which drops dependency on the scikit-learn mixture, but I didn't have time to fully clean it up and test. Would welcome a PR for that.

@chanansh

This comment has been minimized.

chanansh commented Feb 13, 2017

What do you mean by "for that"? Your independent GMMHMM code or for the new sklearn gmm version?

@superbobry

This comment has been minimized.

Member

superbobry commented Feb 13, 2017

Sorry, I should've explained this properly. Even though the master version doesn't rely on GMM it still uses some functions from the removed sklearn.gmm module, see scikit-learn/scikit-learn#7012.

The PR should copy/paste that functions from older scikit-learn or change the code in some other way so that it is compatible with both the newest scikit-learn and a few older versions.

@chanansh

This comment has been minimized.

chanansh commented Feb 13, 2017

@superbobry

This comment has been minimized.

Member

superbobry commented Feb 13, 2017

The problem of the previous implementation was not the dependency on scikit-learn, but rather a bug in the update formulas, see #78. Also, there was a hope that a rewrite would simplify the code (which is not the case right now).

@chanansh

This comment has been minimized.

chanansh commented Feb 13, 2017

@superbobry

This comment has been minimized.

Member

superbobry commented Feb 13, 2017

I am not convinced the rewrite was a good idea, so we might as well reconsider it. What you could try is:

  • checkout the old GMMHMM,
  • update it to use GaussianMixture,
  • apply the fix from #78 and
  • cross-check the resulting implementation against the update eqs here.
@chanansh

This comment has been minimized.

chanansh commented Feb 14, 2017

Hi,
I will need help with that.

Can you create a branch with (1) and (3). I will then add second bullet "update it to use GaussianMixture".

Then we can test if it works.
Thanks!

@chananshgong

This comment has been minimized.

chananshgong commented Feb 26, 2017

hi @superbobry any chance you can create the branch you have suggested and I will move to new GMM version of scikit learn? I will need your guidance if you don't have the time to do it.

@superbobry

This comment has been minimized.

Member

superbobry commented Feb 26, 2017

I think we could avoid branching in the main repo for this issue. You could fork hmmlearn, do the transition in a local branch and then PR it into hmmlearn/hmmlearn. What do you think?

@chananshgong

This comment has been minimized.

chananshgong commented Feb 26, 2017

@superbobry

This comment has been minimized.

Member

superbobry commented Feb 26, 2017

The fix mentioned in #78 was there before the migration, so you can start with d9eabf4 and make it work with the new scikit-learn.

Sorry, I can't be more specific than that since I don't know the new GaussianMixture code.

@chananshgong

This comment has been minimized.

chananshgong commented Feb 26, 2017

@chananshgong

This comment has been minimized.

chananshgong commented Feb 26, 2017

what about "cross-check the resulting implementation against the update eqs here."? What did you mean by that?

@superbobry

This comment has been minimized.

Member

superbobry commented Feb 26, 2017

What tests should I run to check it works?

Running just the unit tests should be OK. However, you can also try learning on a fixed data set using an old model and a new model (with a fixed random seed for initialization) and make sure they converge to the same parameters.

"cross-check the resulting implementation against the update eqs here."? What did you mean by that?

I meant that the update equations (M-step) with the new GaussianMixture should match the ones in the issue. Don't worry about this if the change from GMM to GaussianMixture is trivial.

@chanansh

This comment has been minimized.

chanansh commented Feb 26, 2017

I see tests fails when I pull d9eabf4:

hmmlearn/tests/test_gaussian_hmm.py::TestGaussianHMMWithSphericalCovars::test_bad_covariance_type OMP: Error #100: Fatal system error detected.
OMP: System error #22: Invalid argument
make: *** [test] Aborted (core dumped)
@chananshgong

This comment has been minimized.

chananshgong commented Mar 5, 2017

There seems to be many issues (#183, #182, #178) around the gmm implementation outside of scikit learn. I wanted to help but the tests in the branch you told me to pull from fail. Can you please check and direct me on how to proceed?

@chananshgong

This comment has been minimized.

chananshgong commented Mar 5, 2017

I am stuck now because of:

            warnings.warn("Fitting a GMMHMM with {0!r} covariance type "
                          "is broken in 0.2.0. Please update to 0.2.1 once "
                          "it's available.".format(covariance_type),
                          UserWarning)
@superbobry

This comment has been minimized.

Member

superbobry commented Mar 6, 2017

I am sorry, I have very limited resources at the moment and cannot reply to issues in a timely manner. Maybe a Gitter channel could help the feedback situation...

I see tests fails when I pull d9eabf4.

This is a bit unexpected, because Travis build for this commit was OK. Maybe the issue has to do with OMP and not the test code?

There seems to be many issues (#183, #182, #178) around the gmm implementation outside of scikit learn.

#183 is unrelated to GMMHMM and targets GaussianHMM, the version of GMMHMM used by #182 is unclear and #178 does not look like an implementation issue.

I am stuck now because of: [...]

I think the warning is not relevant, because the ad-hoc fix to this issue was merged in 17f6265.

@chananshgong

This comment has been minimized.

chananshgong commented Apr 3, 2017

I wanted to update that I found a bypass by setting the gmms_ property manually to contain the sklearn class instead of the internal one. One other hack to make it work was to overload the score method of the sklearn class to point to score_samples.
Since this change is very easy I encourage to move to sklearn implementation of gmm instead of an internal one which is less supported by the community.

@tkoch96

This comment has been minimized.

tkoch96 commented Jul 12, 2017

Any update on this?

@brianbolze

This comment has been minimized.

brianbolze commented Nov 13, 2017

Any update on this? Trying to get GMMHMM working with no luck - getting the same scikit-learn depreciation warning.

@manisci

This comment has been minimized.

manisci commented Mar 26, 2018

I'm having the same warnings. Just wanted to say that there is still a lot of demand and appreciation if this issue can somehow be solved. Thanks.

@anntzer

This comment has been minimized.

Contributor

anntzer commented Sep 28, 2018

A complete rewrite of GMMHMM went in with #107, which is not released yet. Closing this issue, please reopen issues against master if needed.

@anntzer anntzer closed this Sep 28, 2018

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