-
-
Notifications
You must be signed in to change notification settings - Fork 26k
added example to decomposition.DictionaryLearning #12799
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
Conversation
…n.DictionaryLearning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please merge master in
>>> import numpy as np | ||
>>> from sklearn.decomposition import DictionaryLearning | ||
>>> X = np.array([[0., 0., 1.], | ||
... [1.,0.,0.], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please avoid flake8 errors in examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flake8 test didn't pick them up. I suppose it is because it is in a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
... [2., 2., 2.], | ||
... [2., 5., 4.]]) | ||
>>> dico = DictionaryLearning(n_components=2, alpha=1, random_state=1) | ||
>>> dico.fit(X).components_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add # doctest: +ELLIPSIS
at the end of this line to make sure the directive is active here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, a little question. What is the purpose of # doctest: +ELLIPSIS
.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it says that ...
can be replaced by any string. The following lines replace the numbers with ...
so that in case of numerical instability, the test still passes. If the directive is not active, doctest would expect exactly that as the output, instead of accepting anything in place of ...
. Not sure if I explained it clearly. reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh yes! thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @reshamas !
Please improve the pull request title. This, by default, becomes the commit message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
... [2., 2., 2.], | ||
... [2., 5., 4.]]) | ||
>>> dico = DictionaryLearning(n_components=2, alpha=1, random_state=1) | ||
>>> dico.fit(X).components_ # doctest: +ELLIPSIS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pep8 there should be two spaces before #
@reshamas usually if there's no merge conflict and the CI on master hasn't changed, you don't need to merge master into your branch for it to get merge into master. |
Hmm, the example is under |
Oops, we missed that. The example should be under the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @reshamas
thank you! @qinhanmin2014 @adrinjalali |
Hmm, DictionaryLearning is not stable, pypy is failing. |
do we know why? |
This also can fail on Mac: https://travis-ci.org/scikit-learn/scikit-learn/jobs/469827638 Seems to be possible to have signs of components flipped... |
Let's revert and open an issue. |
This reverts commit fec534a.
…-learn#12799)" This reverts commit fec534a.
…-learn#12799)" This reverts commit fec534a.
…scikit-learn#12799)"" This reverts commit 9c95965.
…-learn#12799)" This reverts commit 63887d4.
…scikit-learn#12799)"" This reverts commit 9c95965.
…-learn#12799)" This reverts commit 63887d4.
…-learn#12799)" This reverts commit fec534a.
Referencing PR / Issue
This closes #12209
Note
added example to decomposition.DictionaryLearning
cc: @ThaliaBarrera
#wimlds