8000 ENH: Add regularization to the main NMF class by bharatr21 · Pull Request #17414 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

ENH: Add regularization to the main NMF class #17414

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
Merged
merged 12 commits into from
Aug 6, 2020
Merged

ENH: Add regularization to the main NMF class #17414

merged 12 commits into from
Aug 6, 2020

Conversation

bharatr21
Copy link
Contributor
@bharatr21 bharatr21 commented Jun 1, 2020 •
edited
Loading

Reference Issues/PRs

Fixes #12520

What does this implement/fix? Explain your changes.

Introduces the regularization parameter to the main NMF() class instead of just the public non_negative_factorization public function

Any other comments?

Need guidance on how to add tests to the test_nmf_regularization()
Tagging @adrinjalali for PR review, based on the issue

@JosephTLucas JosephTLucas mentioned this pull request Jun 5, 2020
Closed
adrinjalali
adrinjalali reviewed Jul 14, 2020
View reviewed changes
Copy link
Member
@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @Bharat123rox

bharatr21 and others added 2 commits July 14, 2020 17:30
@bharatr21 @adrinjalali
Update _nmf with suggestions from code review
8033bbc 
Update `_nmf.py` with suggestion from review

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
@bharatr21
Refactor tests, fix linter errors
83c3c14
@bharatr21
Copy link
Contributor Author
bharatr21 commented Jul 14, 2020 •
edited
Loading

Doc failures seem to be intermittent, or a build failure from CircleCI which are unrelated to these changes 😞

@bharatr21 bharatr21 requested a review from adrinjalali July 14, 2020 18:49
thomasjpfan
thomasjpfan reviewed Jul 14, 2020
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR @Bharat123rox !

@cmarmo
Copy link
Contributor
cmarmo commented Aug 3, 2020

@Bharat123rox something went wrong in CircleCI, unrelated with your PR: do you mind fixing conflicts in order to trigger the build? Thanks!

cmarmo
cmarmo reviewed Aug 3, 2020
View reviewed changes
Copy link
Contributor
@cmarmo cmarmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Bharat123rox!

@bharatr21 bharatr21 requested a review from cmarmo August 3, 2020 13:25
TomDLT
TomDLT approved these changes Aug 3, 2020
View reviewed changes
Copy link
Member
@TomDLT TomDLT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job !

You might want to add a changelog entry in doc/whats_new/0.24.rst.

thomasjpfan
thomasjpfan reviewed Aug 3, 2020
View reviewed changes
bharatr21
bharatr21 commented Aug 5, 2020
View reviewed changes
@bharatr21 bharatr21 requested review from thomasjpfan and TomDLT August 5, 2020 17:06
thomasjpfan
thomasjpfan approved these changes Aug 6, 2020
View reviewed changes
sklearn/decomposition/_nmf.py
@@ -1081,7 +1081,7 @@ def non_negative_factorization(X, W=None, H=None, n_components=None, *,


class NMF(TransformerMixin, BaseEstimator):
r"""Non-Negative Matrix Factorization (NMF)
"""Non-Negative Matrix Factorization (NMF)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was to make

regularization : {'both', 'components', 'transformation', None}, \
                      default='both'

work with the \. I do not think there is a way to ignore the newline in a raw string.

XREF: numpy/numpydoc#87

@TomDLT TomDLT merged commit f734e11 into scikit-learn:master Aug 6, 2020
@TomDLT
Copy link
Member
TomDLT commented Aug 6, 2020

Thanks @Bharat123rox !

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@bharatr21 @adrinjalali
@thomasjpfan @jayzed82
ENH: Add regularization to the main NMF class (scikit-learn#17414)
11a7fab 
* ENH: Add regularization to the main NMF class

* Update _nmf with suggestions from code review

Update `_nmf.py` with suggestion from review

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

* Refactor tests, fix linter errors

* Change default value to None

* Revert back to default value of "both"

* Update default value documentation acc to @thomasjpfan

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

* CLN Places regularization at the end

* Add whatsnew entry

* DOC Fix

* DOC Fix

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@sadakmed sadakmed sadakmed approved these changes

@adrinjalali adrinjalali Awaiting requested review from adrinjalali

@jeremiedbb jeremiedbb Awaiting requested review from jeremiedbb

@cmarmo cmarmo Awaiting requested review from cmarmo

@TomDLT TomDLT Awaiting requested review from TomDLT

Assignees
No one assigned
Labels
module:decomposition Waiting for Reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

non_negative_factorization's 'regularize' parameter is lost in the OO interface
7 participants
@bharatr21 @cmarmo @TomDLT @adrinjalali @thomasjpfan @sadakmed @jeremiedbb
0