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

moving solvers related code to solvers/* directory, improving fista and nnls_hals #550

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

cohenjer
Copy link
Contributor

@cohenjer cohenjer commented Apr 10, 2024

This is a copy of the part of #542 related to solvers.

What this PR solves:

  • Currently, in Tensorly we have a large chunk of code in tenalg/proximal.py which barely relates to tensor algebra but rather numerical optimization. Other functions in Tensorly perform subtasks for numerical optimization such as nonnegative least squares and admm.
  • The nonnegative least squares solver nnls_hals has a bug related to sparsity (the formula is incorrect). Also, we should support ridge penalization since it is also quite simple.
  • The documentation of nnls_hals and fista was not friendly, and the code was not clear. I tried to improve upon these issues.

What this PR does:

(I left the old proximal file with the modified nnls_hals and fista solvers to show the changes, we can remove it before merging.)

  • Move optimization-related code from various places to a dedicated solvers/ directory. (only copy-pasting files, no changes)
  • Correct the nnls hals solver when sparsity is required and clean up the syntax.
  • Adding support for ridge regularization in fista and in nnls_hals.
  • Reworking documentation for fista and nnls_hals.
  • Update the import paths where it is necessary.
  • Add a module solvers/penalizations.py with a utility function to process input ridge/sparsity regularization (transform 1d input to list of correct length, avoid no regularization on only some modes which makes the factorization ill-posed).

Possible improvements:

  • move the SVD functions also in solvers/ ?

Note: If this PR is merged, I will proceed with the nonnegative algorithms improvements in #542.

@cohenjer cohenjer changed the title moving solvers related code to solver/* directory and updating imports moving solvers related code to solver/* directory, improving fista and nnls_hals, updating imports Apr 10, 2024
@cohenjer cohenjer changed the title moving solvers related code to solver/* directory, improving fista and nnls_hals, updating imports moving solvers related code to solvers/* directory, improving fista and nnls_hals Apr 11, 2024
@cohenjer
Copy link
Contributor Author

cohenjer commented Apr 11, 2024

Not sure why the pytorch test for constrained_parafac is returning an error, I did not change the contents of the code there... seems to be a type mismatch between floats and integers but I have no idea why it happens.

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Attention: Patch coverage is 74.66468% with 170 lines in your changes are missing coverage. Please review.

Project coverage is 80.10%. Comparing base (63fc5c5) to head (4386510).
Report is 6 commits behind head on main.

❗ Current head 4386510 differs from pull request most recent head c553d69. Consider uploading reports for the commit c553d69 to get more accurate results

Files Patch % Lines
tensorly/solvers/proximal.py 67.50% 91 Missing ⚠️
tensorly/solvers/nnls.py 65.07% 44 Missing ⚠️
tensorly/tenalg/proximal.py 0.00% 29 Missing ⚠️
tensorly/solvers/penalizations.py 92.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
- Coverage   87.30%   80.10%   -7.20%     
==========================================
  Files         125      132       +7     
  Lines        7868     8425     +557     
==========================================
- Hits         6869     6749     -120     
- Misses        999     1676     +677     

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

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

1 participant