JuliaStats / NMF.jl

A Julia package for non-negative matrix factorization

Other

90 stars 34 forks source link

Fix #29: Reimplement alternate least square using projected gradient descent #47

Closed ghost closed 3 years ago

ghost commented 3 years ago

I fixed #29 by reimplementing alspgrad.jl as in the following original paper:

Chih-Jen Lin, Projected Gradient Methods for Non-negative Matrix Factorization, Neural Computing, 19 (2007).

Here is the MATLAB code in the original paper. In addition, I fixed the parameters maxiter and tolg in test/alspgrad.jl because they prevented alspgrad.jl from obtaining a high-precision solution.

codecov-io commented 3 years ago

Codecov Report

Merging #47 (007f698) into master (c5e60bf) will decrease coverage by 1.57%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
- Coverage   93.03%   91.46%   -1.58%     
==========================================
  Files          10       10              
  Lines         589      586       -3     
==========================================
- Hits          548      536      -12     
- Misses         41       50       +9

Impacted Files	Coverage Δ
src/alspgrad.jl	`85.14% <100.00%> (-5.31%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c5e60bf...007f698. Read the comment docs.

ghost commented 3 years ago

@andreasnoack, could you please review this PR?

ghost commented 3 years ago

@andreasnoack, could I ask what's blocking from merging this PR? I think fixing #29 makes CI effectively useful.

andreasnoack commented 3 years ago

Hi @tsano430. I'm not really qualified to review this and, unfortunately, I also have too much time for general reviews for the time being. This package hasn't been maintained for a while so your contributions are appreciated. I've invited you to the repo such that you can more easily make progress here. It would be good to continue to let PRs stay open for a couple of days but generally feel free to merge PRs that haven't received any suggestions for changes.

ghost commented 3 years ago

@andreasnoack, thank you for your polite response. I've accepted your invitation to join JuliaStats/NMF.jl. From now on, I will contribute to this package in keeping with your advice.