8000 [MRG+1] Give dpgmm a covars_ property, allows for sampling by amueller · Pull Request #4182 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content
8000

[MRG+1] Give dpgmm a covars_ property, allows for sampling #4182

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[MRG+1] Give dpgmm a covars_ property, allows for sampling #4182

wants to merge 1 commit into from

Conversation

amueller
Copy link
Member

Fixes #1637.

@amueller amueller added the Bug label Jan 29, 2015
@amueller amueller mentioned this pull request Jan 29, 2015
Closed
@amueller amueller added this to the 0.16 milestone Jan 29, 2015
@fluctuation
Copy link

Fixes #4187
Compute covars_ at the end of "fit(X)" method and the sample method can work properly.
example:
dpgmm.fix(X)
newX = dpgmm.sample(100)

@amueller
Copy link
Member Author

Wait, did no-one touch this for two years (and two years to the DAY lol) and then the two of us did a fix within 10 hours?

@amueller
Copy link
Member Author

I'm not 100% certain about property vs attribute. The benefit of the property is that if the user overwrites precs_, all methods will work as expected, where with an attribute as in #4187 the user has to overwrite two attributes for the model to work consistently.

@amueller amueller mentioned this pull request Jan 30, 2015
Closed
@fluctuation
Copy link

Yesterday, i have downloaded sklearn for the DPGMM class which i would like to use in my program. I called the sample method and got an error. After reading the issue #1637 and #2784, i decided to fix it (this is my first proposal in open source project). I have test it by sampling a lot points and calculate the covariances to see if they match the covariances learned by DPGMM).  When i finished submitting following the instructions of git, i found you have fixed it using same approach 10 hours ago!
I think your approach is better. I am new with python. After reading the behavior of property in python, I would like tot use your method instead of attribute. But i would like to compute covar at the end of fit(X) because after calling fit(X), the sample method is ready. This is same as GMM class. Or user need to compute covar_ before using sample(). But the covar_ is not same as the return value of get_covar_() which is so confusing for user.

Please forgive my poor English.

@amueller
Copy link
Member Author
amueller commented Feb 1, 2015

To clarify (not sure if we are on the same page),
the use of the property means that it is computed "on demand",
so with my fix you can do
python DPGMM().fit(X).sample()
which is what you want :)
And I agree that the current behavior is pretty confusing.
Thank you for submitting a fix anyhow, and I hope we see more pull requests coming from you in the future!

@fluctuation
Copy link

I agree with you. I will pull more requests. You are very nice, it's very nice to discuss with you!

@ogrisel
Copy link
Member
ogrisel commented Mar 3, 2015

Sorry I put my comments on a specific commit instead of the PR diff view... Anyway +1 on my side once they are addressed.

@ogrisel ogrisel changed the title [MRG] Give dpgmm a covars_ property, allows for sampling [MRG+1] Give dpgmm a covars_ property, allows for sampling Mar 3, 2015
@ogrisel
Copy link
Member
ogrisel commented Mar 3, 2015

don't forget a what's new entry when merging.

@amueller amueller force-pushed the dpgmm_fix_sampling branch 2 times, most recently from 1a71467 to 3588f71 Compare March 3, 2015 16:30
@amueller amueller force-pushed the dpgmm_fix_sampling branch from 3588f71 to a8dc789 Compare March 3, 2015 16:32
@amueller
Copy link
Member Author
amueller commented Mar 3, 2015

added whatsnew, fixed random state.

@amueller amueller modified the milestones: 0.16.2, 0.16 May 6, 2015
@amueller amueller modified the milestones: 0.16.2, 0.17 Sep 8, 2015
@amueller
Copy link
Member Author

closing in favor of #4802

@amueller amueller closed this Sep 20, 2015
@acidghost
Copy link

I'm probably missing something, but in version 0.17.1 this is still not resolved....

@ogrisel
Copy link
Member
ogrisel commented Sep 6, 2016

This is not resolved in 0.17.1 because it will be tackled in the big refactoring of the Gaussian Mixture models which hopefully will be included in 0.18.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
0.17
Development

Successfully merging this pull request may close these issues.

dpgmm sample not working
4 participants
@amueller @fluctuation @ogrisel @acidghost
0