Skip to content
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

Reworking nonnegative algorithm and solvers #542

Draft
wants to merge 62 commits into
base: main
Choose a base branch
from

Conversation

cohenjer
Copy link
Contributor

@cohenjer cohenjer commented Feb 1, 2024

First of all, I am sorry for the massive PR with so many small commits (to be squashed upon merge hopefully). I have been working on this for quite some time, and cutting this work into pieces seemed artificial and challenging.

Anyways I hope to make the nonnegative things in Tensorly a little cleaner and efficient with this!

Improvements for NNLS

  • Fixed Nonnegative Least Squares (NNLS) solvers (HALS, FISTA) that had bugs in them.
  • Added ridge and sparse regularization support for all NNLS solvers and HALS algorithms.
  • Replaced return_errors with callback for all nonnegative solvers (also Multiplicative Updates solvers). Added documentation which was missing for callback.
  • Added the option to have unconstrained modes in non_negative_tucker_hals (this is now similar to non_negative_parafac_hals API)
  • The error computation (math and code) were different in the various nonnegative decomposition algorithms. I chose to unify them, computing the loss function. This is not what is done in the CP function (we compute the square-root of the loss); this is open to discussion, but showing the loss is what makes the most sense to me. I updated the CP functions as well to have at least the same math for tensor_norm everywhere.
  • Updated API for HALS function to allow some degree of control over the inner HALS nnls solver. But I also simplified the nnls HALS API so these new inputs should be self-explanatory.
  • Reworked documentation (a lot) to add some missing arguments, and improve clarity.
  • Updated the nonnegative decomposition examples to make them easier to follow and simpler. Also they now explain random tensor generations and parameter tuning in the algorithms.
  • Of course, modified the tests where needed, and added new ones for the solvers/ (see below)

Overall improvements

  • Reverted MTTKRP back to the faster matrix-matrix product. I added the current memory-efficient version too, with some snippet of code in the documentation to show how to switch from one to the other using the backend system. This makes all CP algorithms significantly faster as we discussed here The unfolding_dot_khatri_rao-function is unneccesarily slow #442
  • Created a solvers/ folder in tensorly, to move all the optimization methods from tenalg/ to a dedicated place. solvers/ has four modules (nnls.py, admm.py, proximal.py and penalizations.py, this last one being mostly empty but I plan to populate it in a coming PR).

Others

  • Adding the maximum() function in the backend, useful for nonnegative projections.

Pending

  • The FISTA algorithm in HALS for nonnegative Tucker needs a stepsize, right now I use some hand-crafted scheduling (compute the Lipschitz constant for first 20 outer iterations which are critical, then update every 20 iterations). Is this satisfactory? If yes I can add some documentation for this.
  • There are some tests which are not passing, but I am unsure if this is due to bugs introduced by this PR (probably not). One test however that failed after changing back the MTTKRP to the matrix-matrix product version is test_entropy.py. I had to comment out the test with normalization activated, I have no explanation as to why that failed...

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Attention: 104 lines in your changes are missing coverage. Please review.

Comparison is base (2a8ff56) 87.15% compared to head (3240db7) 86.85%.

Files Patch % Lines
tensorly/solvers/nnls.py 65.87% 43 Missing ⚠️
tensorly/decomposition/_tucker.py 59.80% 41 Missing ⚠️
tensorly/decomposition/_nn_cp.py 82.60% 12 Missing ⚠️
tensorly/solvers/penalizations.py 92.00% 6 Missing ⚠️
tensorly/backend/core.py 66.66% 1 Missing ⚠️
tensorly/tucker_tensor.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #542      +/-   ##
==========================================
- Coverage   87.15%   86.85%   -0.30%     
==========================================
  Files         121      127       +6     
  Lines        7693     7937     +244     
==========================================
+ Hits         6705     6894     +189     
- Misses        988     1043      +55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JeanKossaifi
Copy link
Member

@cohenjer The changes sound amazing, and went through it quickly, looks good on a high level.

I know you mentioned it feels artificial to break into multiple PRs, but breaking down the PR into smaller, more manageable PRs might make reviewing easier.

Docs, examples etc can be easily enough reviewed as is I think so maybe we can split novel additions first.
Otherwise, we can try to review as is. Maybe @aarmey and others would be able to have a look too?

@aarmey
Copy link
Contributor

aarmey commented Mar 12, 2024

@cohenjer this is outstanding! Many thanks.

I agree about splitting this up. For instance, perhaps much of the (1) documentation, (2) return_errors interface, (3) MTTKRP, and (4) tl.maximum changes can be separate PRs? I'm happy to quickly review each if so. It's so easy to miss bugs otherwise...

I am strongly in favor of using a consistent loss throughout (and having it be the loss, not the square root)! This inconsistency has tripped me up several times.

@cohenjer
Copy link
Contributor Author

Hi @JeanKossaifi @aarmey. Thank you for the kind comments, and very happy to hear you think the changes are positive! I agree the PR is too large to be reviewed efficiently; I will cut it down into several parts (possibly more than four). However, I am unsure if there is a more efficient process than copy/paste the new content manually. My commits are all over the place :/

@aarmey
Copy link
Contributor

aarmey commented Mar 12, 2024

One method that has helped me is to make a new branch, then git checkout old_branch -- file_path. This way you can copy over a file and changes to a new branch without having to manually copy and paste.

@JeanKossaifi
Copy link
Member

Great suggestion @aarmey - we should have a section in the contribution doc to collate all these tips!

@cohenjer
Copy link
Contributor Author

Working on this right now, should be creating a few PRs soon !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants