add multi Y options to nipals #122
Conversation
Codecov Report
@@ Coverage Diff @@
## master #122 +/- ##
=======================================
Coverage 66.47% 66.47%
=======================================
Files 32 32
Lines 3147 3147
Branches 453 453
=======================================
Hits 2092 2092
Misses 975 975
Partials 80 80
Continue to review full report at Codecov.
|
|
Could you add an explanation about why exactly you want this change ? I don't see difference between. before and after. |
|
First I removed this part of the code that disables the possibility of an Y with more than one column. error message for when the maximum number of iterations is reached the normalization of loadings as the source code and the testcase to compare the results of both codes: |
| @@ -18,22 +18,15 @@ export default class nipals { | |||
| } else { | |||
| Y = WrapperMatrix2D.checkMatrix(Y); | |||
| } | |||
| if (!Y.isColumnVector() || Y.rows !== X.rows) { | |||
There was a problem hiding this comment.
I would leave the test, just adapt it so it works for multiple Y
| @@ -18,22 +18,15 @@ export default class nipals { | |||
| } else { | |||
| Y = WrapperMatrix2D.checkMatrix(Y); | |||
| } | |||
| if (!Y.isColumnVector() || Y.rows !== X.rows) { | |||
| throw new Error('Y must be a column vector of length X.rows'); | |||
There was a problem hiding this comment.
adapt the text of the error message as well to reflect the results of the new test
src/dc/nipals.js
Outdated
|
|
||
| if (Y) { | ||
| let p = X.transpose().mmul(t).div(t.transpose().mmul(t).get(0, 0)); | ||
| p = p.div(p.norm()); |
There was a problem hiding this comment.
I would leave the normalisation as before, I think this is wrong in the reference code
No description provided.