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] ENH: Optional positivity constraints on the dictionary and sparse code #6374

Merged
merged 3 commits into from Jun 21, 2018

Conversation

Projects
None yet
7 participants
@jakirkham
Contributor

jakirkham commented Feb 17, 2016

Allows for a positivity constraint to be set during dictionary learning. This should be very similar to one provided for by Marial, et al. in SPAMS. However, I may need guidance in making sure this is working correctly in all cases.

Todo:

  • Positivity constraint for dictionary.
    • Provide support for positivity constraint on dictionary.
    • API documentation for using positivity constraint.
    • Tests demonstrating the positivity constraint works correctly.
  • Positivity constraint for sparse code.
    • Provide support for positivity constraint on sparse code.
    • API documentation for using positivity constraint.
    • Raise exception when positivity constraint is set incorrectly (when using orthogonal matching pursuit).
    • Tests demonstrating the positivity constraint works correctly.
  • Add an example of using this functionality.
@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux Feb 17, 2016

Member

In sparse_encode, you cannot ignore the positive argument for algorithms that do not support it. You need either to support it (for threshold it's just a question of putting the negative values to zero) or to raise an error.

In addition, you need a test for all the features that you have added, here testing positive support for all algorithms.

Member

GaelVaroquaux commented Feb 17, 2016

In sparse_encode, you cannot ignore the positive argument for algorithms that do not support it. You need either to support it (for threshold it's just a question of putting the negative values to zero) or to raise an error.

In addition, you need a test for all the features that you have added, here testing positive support for all algorithms.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Feb 17, 2016

Contributor

Added positivity support for the threshold case and added a ValueError for the orthogonal matching pursuit case.

Added tests that verify the positivity constraint is met for the sparse code in a variety of cases. Also, verifies that the ValueError for the orthogonal matching pursuit case is raised appropriately.

Additionally renamed the positive argument to be more clear in some cases. This constraint ended up needing to be included as part of the SparseCodingMixin to get the right behavior from transform.

It would be nice to extend this to the dictionary, as well. This is something that Marial, et al. do in SPAMS. I know this will come down to some sort of thresholding on the dictionary, but am a little unclear as to where. If you have any thoughts, please share.

Contributor

jakirkham commented Feb 17, 2016

Added positivity support for the threshold case and added a ValueError for the orthogonal matching pursuit case.

Added tests that verify the positivity constraint is met for the sparse code in a variety of cases. Also, verifies that the ValueError for the orthogonal matching pursuit case is raised appropriately.

Additionally renamed the positive argument to be more clear in some cases. This constraint ended up needing to be included as part of the SparseCodingMixin to get the right behavior from transform.

It would be nice to extend this to the dictionary, as well. This is something that Marial, et al. do in SPAMS. I know this will come down to some sort of thresholding on the dictionary, but am a little unclear as to where. If you have any thoughts, please share.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Feb 18, 2016

Contributor

So, I have added something that I think will correctly constrain the dictionary to positive values. It does appear to work. However, it may not be the best way or could have errors so feedback is definitely welcome here. Still not sure if there isn't something missing here.

Contributor

jakirkham commented Feb 18, 2016

So, I have added something that I think will correctly constrain the dictionary to positive values. It does appear to work. However, it may not be the best way or could have errors so feedback is definitely welcome here. Still not sure if there isn't something missing here.

@jakirkham jakirkham changed the title from ENH: Allow for a positivity constraint in dictionary learning methods to WIP, ENH: Allow for a positivity constraint in dictionary learning methods Feb 18, 2016

@jakirkham jakirkham changed the title from WIP, ENH: Allow for a positivity constraint in dictionary learning methods to ENH: Allow for a positivity constraint in dictionary learning methods Feb 18, 2016

@jakirkham jakirkham changed the title from ENH: Allow for a positivity constraint in dictionary learning methods to ENH: Allow for a positivity constraints on the dictionary and sparse code Feb 18, 2016

@jakirkham jakirkham changed the title from ENH: Allow for a positivity constraints on the dictionary and sparse code to ENH: Optional positivity constraints on the dictionary and sparse code Feb 18, 2016

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Feb 18, 2016

Contributor

Thus far, using this function on more complex datasets seems to yield the right results.

Contributor

jakirkham commented Feb 18, 2016

Thus far, using this function on more complex datasets seems to yield the right results.

@arthurmensch

This comment has been minimized.

Show comment
Hide comment
@arthurmensch

arthurmensch Feb 18, 2016

Contributor

We need an example showing the interest of enforcing positivity to the dictionary / code. It would be great to compare to NMF estimators. I guess modifying faces decomposition examples is the easiest way, though it would be interesting to have a different usecase.

Contributor

arthurmensch commented Feb 18, 2016

We need an example showing the interest of enforcing positivity to the dictionary / code. It would be great to compare to NMF estimators. I guess modifying faces decomposition examples is the easiest way, though it would be interesting to have a different usecase.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Feb 18, 2016

Contributor

So, I am using this in image processing of calcium image data from the mouse brain primarily (various regions). While there are many cases of using NMF in this area (and we tried ourselves), we found dictionary learning with positivity constraints was more performant. Even on artificial data (used for testing), I will get the wrong answer without this positivity constraint on the dictionary.

The reason I added this is it was a feature provided in SPAMS by Marial, et al.. However, this package has gone without a release for a year and a half. I have tried patching it to fix various bugs that have crept up (NumPy support, better linking to the BLAS, etc.), but this solution is untenable. Especially as I am now looking to have Python 3 support and a new version of NumPy is on the horizon. It seems the original authors are likely exploring new interesting problems, which makes sense. Unfortunately, this is a problem from the maintenance side. I suppose one could argue about licensing too, but this seems secondary to having a working dependency stack.

Contributor

jakirkham commented Feb 18, 2016

So, I am using this in image processing of calcium image data from the mouse brain primarily (various regions). While there are many cases of using NMF in this area (and we tried ourselves), we found dictionary learning with positivity constraints was more performant. Even on artificial data (used for testing), I will get the wrong answer without this positivity constraint on the dictionary.

The reason I added this is it was a feature provided in SPAMS by Marial, et al.. However, this package has gone without a release for a year and a half. I have tried patching it to fix various bugs that have crept up (NumPy support, better linking to the BLAS, etc.), but this solution is untenable. Especially as I am now looking to have Python 3 support and a new version of NumPy is on the horizon. It seems the original authors are likely exploring new interesting problems, which makes sense. Unfortunately, this is a problem from the maintenance side. I suppose one could argue about licensing too, but this seems secondary to having a working dependency stack.

@arthurmensch

This comment has been minimized.

Show comment
Hide comment
@arthurmensch

arthurmensch Feb 18, 2016

Contributor

I am sure there are some usecases where it performs better than NMF. However there is no point in providing a new functionality to the end-user without advertising it by an example. Do you have any idea ? I'll look into that as well

Contributor

arthurmensch commented Feb 18, 2016

I am sure there are some usecases where it performs better than NMF. However there is no point in providing a new functionality to the end-user without advertising it by an example. Do you have any idea ? I'll look into that as well

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Feb 18, 2016

Contributor

Ok, sure, I'll try to come up with an example. It might not be today, but I will at least try tomorrow or this weekend.

Contributor

jakirkham commented Feb 18, 2016

Ok, sure, I'll try to come up with an example. It might not be today, but I will at least try tomorrow or this weekend.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Feb 22, 2016

Contributor

For an example, take a look at this ( http://nbviewer.jupyter.org/gist/jakirkham/c5622e41843e6dafbcb7 ). I modified an example that I found in the scikit-learn docs and imposed the positivity constraints in different combinations. Thoughts?

Contributor

jakirkham commented Feb 22, 2016

For an example, take a look at this ( http://nbviewer.jupyter.org/gist/jakirkham/c5622e41843e6dafbcb7 ). I modified an example that I found in the scikit-learn docs and imposed the positivity constraints in different combinations. Thoughts?

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham May 10, 2016

Contributor

Any thoughts on the example, @arthurmensch? I would really like to find a way forward here, but am unclear as to what it is at this point.

Contributor

jakirkham commented May 10, 2016

Any thoughts on the example, @arthurmensch? I would really like to find a way forward here, but am unclear as to what it is at this point.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Oct 3, 2016

Contributor

Any more thoughts on this?

Contributor

jakirkham commented Oct 3, 2016

Any more thoughts on this?

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Oct 7, 2016

Member

Sorry for our slow replies @jakirkham. I think we don't have maintainers that use these methods a lot unfortunately, and we are swamped with many PRs. We need to figure something out though :-/

Member

amueller commented Oct 7, 2016

Sorry for our slow replies @jakirkham. I think we don't have maintainers that use these methods a lot unfortunately, and we are swamped with many PRs. We need to figure something out though :-/

@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Nov 1, 2016

Member

Ping @vene

Member

jnothman commented Nov 1, 2016

Ping @vene

@vene

This comment has been minimized.

Show comment
Hide comment
@vene

vene Nov 3, 2016

Member

(Caveat: I don't really have any experience with practical application of dictionary learning whatsoever...)

Maybe just show the last slice of the example (positive dictionary + code) since it looks like the components are a bit sparser than NMF.

Another idea is to check if it works as a topic model for text data, comparably to how NMF does.

Member

vene commented Nov 3, 2016

(Caveat: I don't really have any experience with practical application of dictionary learning whatsoever...)

Maybe just show the last slice of the example (positive dictionary + code) since it looks like the components are a bit sparser than NMF.

Another idea is to check if it works as a topic model for text data, comparably to how NMF does.

@ogrisel

ogrisel approved these changes Jun 1, 2018 edited

LGTM but disclaimer: I am not familiar with the SPAMS implementation. Maybe @arthurmensch is more knowledgeable.

It would be great to update the faces decomposition example with a new entry with a positivity constraint on the dictionary components.

Show outdated Hide outdated sklearn/decomposition/dict_learning.py Outdated
Show outdated Hide outdated sklearn/decomposition/tests/test_dict_learning.py Outdated
Show outdated Hide outdated sklearn/decomposition/tests/test_dict_learning.py Outdated
@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Jun 2, 2018

Contributor

Thanks for your feedback, @ogrisel.

Have pushed some changes that hopefully address your comments and have squashed the history a bit. Tests appear to be passing. 🎉

For the most part this is just relying on whatever algorithm exposed positivity constraint is available and exposing that to the user. With the exception of the thresholding case where it is custom and orthogonal matching pursuit where we just error out (as it is unsupported), which is just following @GaelVaroquaux's advice earlier in the thread. So would hope knowledge of SPAMS is not required, but it would be nice to have someone with that expertise take a look, if they have time.

As to an example (though we did look at this case in person), have the faces with the positivity constraint in a notebook. This shows a few different results depending on these constraints. Would be happy to add them if they would be useful. Could also play with different data like the digits set as you suggested or something else that we deem appropriate. Though would suggest either selecting one or two of these to add to the existing gallery or breaking them out on another doc page to avoid having too much info for users to ingest at one time.

Thoughts on any/all of this would be welcome. 😄

Contributor

jakirkham commented Jun 2, 2018

Thanks for your feedback, @ogrisel.

Have pushed some changes that hopefully address your comments and have squashed the history a bit. Tests appear to be passing. 🎉

For the most part this is just relying on whatever algorithm exposed positivity constraint is available and exposing that to the user. With the exception of the thresholding case where it is custom and orthogonal matching pursuit where we just error out (as it is unsupported), which is just following @GaelVaroquaux's advice earlier in the thread. So would hope knowledge of SPAMS is not required, but it would be nice to have someone with that expertise take a look, if they have time.

As to an example (though we did look at this case in person), have the faces with the positivity constraint in a notebook. This shows a few different results depending on these constraints. Would be happy to add them if they would be useful. Could also play with different data like the digits set as you suggested or something else that we deem appropriate. Though would suggest either selecting one or two of these to add to the existing gallery or breaking them out on another doc page to avoid having too much info for users to ingest at one time.

Thoughts on any/all of this would be welcome. 😄

@ogrisel

This comment has been minimized.

Show comment
Hide comment
@ogrisel

ogrisel Jun 6, 2018

Member

@jakirkham yes! Please add the MBDL case with positive code and dict to the existing example for the faces decomposition. Maybe using the red-blue color map with white centered at zero for all those face plots would better highlight the differences between the decompositions.

Member

ogrisel commented Jun 6, 2018

@jakirkham yes! Please add the MBDL case with positive code and dict to the existing example for the faces decomposition. Maybe using the red-blue color map with white centered at zero for all those face plots would better highlight the differences between the decompositions.

@ogrisel

This comment has been minimized.

Show comment
Hide comment
@ogrisel

ogrisel Jun 6, 2018

Member

Maybe breaking out the positivity constraints on a simpler dataset like digits in a standalone example is a good idea too. I let you experiment and judge which is best.

Member

ogrisel commented Jun 6, 2018

Maybe breaking out the positivity constraints on a simpler dataset like digits in a standalone example is a good idea too. I let you experiment and judge which is best.

@ogrisel

This comment has been minimized.

Show comment
Hide comment
@ogrisel

ogrisel Jun 6, 2018

Member

Also, I am wondering if it would not be better to rename "dict_positive" to "positive_dict" and "code_positive" to "positive_code". You are the native English speaker though :)

Member

ogrisel commented Jun 6, 2018

Also, I am wondering if it would not be better to rename "dict_positive" to "positive_dict" and "code_positive" to "positive_code". You are the native English speaker though :)

jakirkham added some commits Jun 11, 2018

ENH: Add positivity option for code and dictionary
Provides an option for dictionary learning to positively constrain the
dictionary and the sparse code. This is useful in applications of
dictionary learning where the data is know to be positive (e.g. images),
but the sparsity constraint that dictionary learning has is better
suited for factorizing the data in contrast to other positively
constrained factorization techniques like NMF, which may not be
similarly sparse.
TST: Test positivity with code and dictionary
Ensure that when the positivity constraint is applied that the
dictionary and code end up having only positive values in the respective
results depending on whether dictionary and/or code are positively
constrained.
DOC: Positivity constraints dictionary learning
Shows the various positivity constraints on dictionary learning and what
the results of these look like using a Red to Blue color map. These are
included in the examples and also in the docs below dictionary learning.
All of these use the Olivetti faces as a training set.
@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Jun 11, 2018

Contributor

Thanks for the suggestions, @ogrisel.

Agree that positive_dict/positive_code sounds better. Have updated the API accordingly.

The red/blue color map with faces is a good idea. Have included some code for this in plot_faces_decomposition.py in a separate section. Also have included these images in the docs under the "Generic dictionary learning" section with some text beforehand.

Please let me know what you think. :)

Contributor

jakirkham commented Jun 11, 2018

Thanks for the suggestions, @ogrisel.

Agree that positive_dict/positive_code sounds better. Have updated the API accordingly.

The red/blue color map with faces is a good idea. Have included some code for this in plot_faces_decomposition.py in a separate section. Also have included these images in the docs under the "Generic dictionary learning" section with some text beforehand.

Please let me know what you think. :)

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Jun 13, 2018

Contributor

Have added a screenshot of what this doc section looks like below.

Dictionary Learning Docs
Contributor

jakirkham commented Jun 13, 2018

Have added a screenshot of what this doc section looks like below.

Dictionary Learning Docs

@jakirkham jakirkham changed the title from ENH: Optional positivity constraints on the dictionary and sparse code to [MRG] ENH: Optional positivity constraints on the dictionary and sparse code Jun 13, 2018

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Jun 18, 2018

Contributor

Any other thoughts?

Contributor

jakirkham commented Jun 18, 2018

Any other thoughts?

@ogrisel ogrisel merged commit 6ce497c into scikit-learn:master Jun 21, 2018

8 checks passed

LGTM analysis: Python No alert changes
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: python2 Your tests passed on CircleCI!
Details
ci/circleci: python3 Your tests passed on CircleCI!
Details
codecov/patch 98.3% of diff hit (target 95.24%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +3.06% compared to 1480e9f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ogrisel

This comment has been minimized.

Show comment
Hide comment
@ogrisel

ogrisel Jun 21, 2018

Member

Thank you very much @jakirkham! That is a great contribution.

Member

ogrisel commented Jun 21, 2018

Thank you very much @jakirkham! That is a great contribution.

@ogrisel

This comment has been minimized.

Show comment
Hide comment
@ogrisel

ogrisel Jun 21, 2018

Member

I merged a bit too quickly: I forgot to ask for an entry in whats_new.rst and we also need the .. versionadded:: 0.20 annotation for the new options in the docstrings of the public functions / methods.

Member

ogrisel commented Jun 21, 2018

I merged a bit too quickly: I forgot to ask for an entry in whats_new.rst and we also need the .. versionadded:: 0.20 annotation for the new options in the docstrings of the public functions / methods.

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Jun 21, 2018

Contributor

Thanks @ogrisel. Glad to have this in. 😄

No worries. Does PR ( #11341 ) address this?

Contributor

jakirkham commented Jun 21, 2018

Thanks @ogrisel. Glad to have this in. 😄

No worries. Does PR ( #11341 ) address this?

@jakirkham jakirkham deleted the jakirkham:dict_pos_constrt branch Jun 21, 2018

@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux Jun 21, 2018

Member
Member

GaelVaroquaux commented Jun 21, 2018

@jakirkham

This comment has been minimized.

Show comment
Hide comment
@jakirkham

jakirkham Jun 22, 2018

Contributor

Of course. Thanks for the help reviewing.

Contributor

jakirkham commented Jun 22, 2018

Of course. Thanks for the help reviewing.

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