markrogoyski / math-php

Powerful modern math library for PHP: Features descriptive statistics and regressions; Continuous and discrete probability distributions; Linear algebra with matrices and vectors, Numerical analysis; special mathematical functions; Algebra
MIT License
2.33k stars 240 forks source link

Svd #421

Closed Beakerboy closed 3 years ago

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.002%) to 99.932% when pulling e36a384656fe7e15f31e1e1280d904cf57a917a5 on Beakerboy:SVD into a66f9ad6deeef1f7b8e99a05f41f11c52d5609a5 on markrogoyski:develop.

Beakerboy commented 3 years ago

In the earlier discussion (#338), you had a test for the matrix:

[[1,1,1,1,1]
 [1,1,1,1,1]
 [1,1,1,1,1]]

and were worried that the getV()->isOrthogonal() test failed. It looks like the V matrix will only be orthogonal if the input matrix is full rank. Since Gaussian elimination of the source matrix will have two rows of all zeros, it is not full rank, I could add your case as a test, and only assert that it is orthogonal if the rank equals the number of rows if you would like.

markrogoyski commented 3 years ago

In the earlier discussion (#338), you had a test for the matrix:

[[1,1,1,1,1]
 [1,1,1,1,1]
 [1,1,1,1,1]]

and were worried that the getV()->isOrthogonal() test failed. It looks like the V matrix will only be orthogonal if the input matrix is full rank. Since Gaussian elimination of the source matrix will have two rows of all zeros, it is not full rank, I could add your case as a test, and only assert that it is orthogonal if the rank equals the number of rows if you would like.

I recommend making it a separate test case and not having unnecessary logic in unit tests.

markrogoyski commented 3 years ago

Thanks for following up with this pull request!

markrogoyski commented 3 years ago

Thanks for following up with this.

I tried a lot of test cases and they all seemed to work. The only problem I encountered was the 1x1 [[0]] matrix.

Beakerboy commented 3 years ago

Fixed comments:

Beakerboy commented 3 years ago

I believe all your comments have been addressed.

markrogoyski commented 3 years ago

Thanks. This is great. SVD can enable a lot of other matrix functionality. Thanks for following up.