add nipals loop for factorization #91
Conversation
Codecov Report
@@ Coverage Diff @@
## master #91 +/- ##
========================================
+ Coverage 59.16% 60% +0.83%
========================================
Files 31 32 +1
Lines 3044 3095 +51
Branches 416 428 +12
========================================
+ Hits 1801 1857 +56
+ Misses 1140 1136 -4
+ Partials 103 102 -1
Continue to review full report at Codecov.
|
|
with this the tests are as deep as possible... |
src/dc/nipals.js
Outdated
| } | ||
|
|
||
| counter++; | ||
| if (counter > 1000) break; |
There was a problem hiding this comment.
I think this should be an option with default value 1000. Not nice to have an hardcoded value in the code.
There was a problem hiding this comment.
The code has been modified as suggested. It is now an option.maxIterations with default value set to 1000
src/dc/nipals.js
Outdated
| } else { | ||
| return { t, | ||
| w, | ||
| residual: X.clone().sub(t.clone().mmul(w.transpose())) }; |
There was a problem hiding this comment.
mmul creates always a new matrix so it it useless to clone in before.
There was a problem hiding this comment.
code has been modified as suggested
src/dc/nipals.js
Outdated
| let t, q, w, tOld; | ||
|
|
||
| let counter = 0; | ||
| for (var i = 0; diff > 1e-10; i++) { |
There was a problem hiding this comment.
Should also be an options with default value
There was a problem hiding this comment.
code has been updated as suggested: options.terminationCriteria has been set with default to 1e-10
src/dc/nipals.js
Outdated
| /** | ||
| * NIPALS | ||
| * @param {Matrix} X | ||
| * @param {*} Y |
There was a problem hiding this comment.
Y is a number ? Should be {number}. Small explanation of the option would be nice
There was a problem hiding this comment.
Y is a column vector to project onto X. If Y is passed then NIPALS works as a PLS instead of a PCA. The help has been updated accordingly.
src/dc/nipals.js
Outdated
| let t, q, w, tOld; | ||
|
|
||
| let counter = 0; | ||
| for (var i = 0; diff > 1e-10; i++) { |
There was a problem hiding this comment.
you don't actually use this i variable. It seems to me that you should instead increment the counter here.
for (var counter = 0; diff > 1e-10 && counter < 1000; counter++)
There was a problem hiding this comment.
the conditions for the loop have been modified accordingly
data/irisDataset.json
Outdated
| @@ -0,0 +1,152 @@ | |||
| [[5.1, 3.5, 1.4, 0.2, "setosa"], | |||
There was a problem hiding this comment.
could you please remove this file and use ml-dataset-iris instead?
There was a problem hiding this comment.
It would be:
npm install -D ml-dataset-iris
import { getNumbers } from 'ml-dataset-iris';
const rawData = getNumbers();There was a problem hiding this comment.
the code has been modified accordingly
this decomposition is needed for pca, pls and opls