search

wellposed / hblas

haskell bindings for blas and lapack
www.wellposed.com
BSD 3-Clause "New" or "Revised" License
49 stars 19 forks source link

Add symm #32

Closed yangjueji closed 9 years ago

yangjueji commented 9 years ago

Add basic symm operation, and its test.

cartazio commented 9 years ago

a few questions/feedback (and I may very well be overloooking something)

1) why do some of the test descriptions say gemm when they're clearly testing a symm operation?

2) i notice that you're using the generateMutableDenseMatrix for testing the symm (symmetric) matrices, I think that perhaps you might want instead want two helpers like 

generateMutableUpperTriangular orient tup const 
        = generateMutableDenseMatrix orient tup (\(x,y)-> if x >= y then const else 0)

and

generateMutableLowerTriangular orient tup const
     = generateMutableDenseMatrix orient tup (\(x,y)-> y >= x then const else 0)

that way you can make sure you're actually doing the symmetric matrix product.

3) likewise, i dont know if you need to cover all those combinations in the test suite, but having more coverage is always a good thing! (have you ever tried out using the haskell coverage checking tools? @ttuegel is an expert at them)

yangjueji commented 9 years ago

I was trying to use Test.QuickCheck to check some properties like "result of gemm and symm is equal when the matrix is both upper and lower triangular". I wrote SymmetricMatrix as an Test.QuickCheck.Arbitrary to generate random symmetric matrix. All my QuickCheck tries are over Matrix.DenseMatrix which derive from Show, but I found that all the computations are over the Matrix.MutableDenseMatrix which has no Show function except the comments (Numerical/HBLAS/MatrixTypes.hs: Line 213-Line221).

It is my first time to use Test.QuickCheck. I will work further to make Matrix.MutableDenseMatrix to have Show function(easy to see whether I am using QuickCheck in a correct way.) and then add properties check.

I think it is a more long-term way to cover all those combinations.

yangjueji commented 9 years ago

It seems not impossible to add show function for Matrix.MutableDenseMatrix.

Because Data.Vector.Storable.Mutable is not Showable. And the SM.read/unsafeRead return m a not a.

cartazio commented 9 years ago

You can't really define show for mutable data structures. Well, not in a way that can be a good show instance. Why do you need one here?

If you want to see a decent preexisting linear algebra test suite, hmatrix has one. Don't worry about quick check for now. View the unit tests here as 1-2 examples for each function that check that it's giving us the desired answer. We can work on fancier testing later. The reason the gemm code has so many tests is because it was needed to track down a bug over there.

cartazio commented 9 years ago

Ohhhhh. I see. Hrmmm. I'll look again this evening. Maybe include s comment explaining that property your trying to test

yangjueji commented 9 years ago

For "View the unit tests here as 1-2 examples for each function that check that it's giving us the desired answer. ", "The reason the gemm code has so many tests is because it was needed to track down a bug over there.". In test, given so many gemm tests:

  1. should we check every function in both row oriented and column oriented?
  2. should we check every [s/d/c/z] function for certain operation?
  3. should we check both square matrix and non-square matrix for every input paramter?
cartazio commented 9 years ago

very good question. the bug i'm thinking about was the one fixed by the following commit https://github.com/wellposed/hblas/commit/b5f2afb893afb79426a93477ecd2b867a35cbc28

see the discussion on this PR https://github.com/wellposed/hblas/pull/25

i think we may in fact be able to shrink the number of tests. Or maybe, as you imply, some engineering (not this PR) should be done to provide some quickcheck machinery for testing hblas a bit more simply!

i'll do a second read through of your tests again tomorrow and then I should have a clearer idea. (you're raising some great points though)

cartazio commented 9 years ago

ok, this looks good.

cartazio commented 9 years ago

theres probably some room for more polish, but this is lovely!

cartazio commented 9 years ago

as for the concern about testing, at some level it comes down to "are we testing the ffi wrapper, or the underlying library"?

It might be worth thinking about ways to support quickcheck testing. but that is its own subproject

cartazio commented 9 years ago

congrats its merged in! I forgot to ask: i assume all work is your own, and you're happy with making it available under a BSD/MIT license? (never hurts to ask explicitly :) )

yangjueji commented 9 years ago

Yes, all work is my own. Sure, I am fine with the license. :)