Pull Request For Issue #14

[AdLucem]

Please review the tests?

[AdLucem]

Sorry, my bad! Removed .stack_work and stack.yaml.

[bollu]

BTW, Please add a closes #14 in the pull request description to make sure that the issue is automatically closed when the PR is merged :) That simplifies managing issues.

[ocramz]

Thank you @AdLucem for following up on this and @bollu for the thorough review. All contributions including code review are certainly welcome.

Currently there is a test error related to floating point comparison :

      test/LibSpec.hs:18: 
      1) Q-R Decomposition Matrix Q is orthogonal
           expected: True
            but got: False

[AdLucem]

@ocramz That's not a floating point error, I checked. Matrix Q is not orthogonal (orthogonal under the definition of matrix * matrix transpose = identity matrix) I commented out the test print in test/LibSpec.hs showing matrix Q for my test case (tried it for two separate matrices), but you can uncomment it and check.

[Shimuuar]

And I strongly susoect it is floating point rounding errors. There are two possible sourses of errors. 1) QR factorization itself 2) Q maybe not representable as orthogonal matrix and IEEE754 approximation may be not orthogonal

[AdLucem]

@Shimuuar Oh. I'm not sure, so I'll just write the example I tested here: For a test matrix, this was the matrix Q I got (the other matrix was an upper triangular matrix):

0.0000 0.9129 -0.9631 0.4472 0.3651 -0.2408 0.8944 -0.1826 0.1204

Calculating Q * (Q transpose), this is what I got:

1.7609 0.5652 -0.2826 0.5652 0.3912 0.3043
-0.2826 0.3043 0.8477

AFAIK it's not close enough to an identity matrix for it to be a floating point error, but I might be wrong?

[Shimuuar]

THAT is clearly bug in algorithm (or in test). Values of the order 1E-15 could and will arise due to rounding. But not valus of the order 1

[AdLucem]

@Shimuuar the result matrix above is hand-calculated (using a matrix calculator), not from the tests.

[Shimuuar]

One thing. When this PR will be merged please use "squash and merge". Otherwise *.o* &*.hi` files will be left in history and they're quite bulky

[ocramz]

@AdLucem will you have time to complete this? I think it's just a matter of picking a different test matrix. The zeros on the diagonal might be the problem.

[AdLucem]

@ocramz I tried test matrices without zeroes on the diagonal and it gave the same problem... I'll try fixing up some display-tests to maybe conclusively show what's going wrong?

[ocramz]

@AdLucem yes please!

[AdLucem]

@AdLucem could you remove the .stack-work folder from the PR ?

Ah, will do. I figured out what was wrong (thanks to @bollu ) and was correcting it

[AdLucem]

@marcozocca-seal I fixed the test issue. Sort of.

• First of all, I am really sorry for the amount of delay it took me to fix changes in this.
• The tests work. Er, all three of them. I also wrote a general scaffold (basically, "here is where you put these functions"), for further tests.
• The issue was that QR factorization doesn't work for non-invertible matrices, which is something I had mistakenly assumed that the algorithm would check for. I fixed this within my tests by explicitly mentioning that "matrix Q is orthogonal for an invertible matrix", and testing on manually-written invertible matrices. Technically I should test on randomized generated matrices of large size, but... well, this leads to my next point:
• I wrote a decent-sized "Utils.hs" file for basic matrix functions- getting determinant, checking if matrices are orthogonal, etc... but these functions should, by rights, be in the main linear algebra library. I'm pushing it here for now but.
• Also the Approximate Comparisions Monster finally reared it's ugly head. I have ad-hoc solved it by using the Decimal library that lets you convert Double to a Decimal with any given precision, and writing a few functions (also in Utils.sh), that let you compare numbers (and matrices) with an arbitrary, tester-provided precision.
• The CI tests seem to be failing because of a dependency issue in the larger library (datasets/, to be specific). Could you look at that?