jblas-project / jblas

Linear Algebra for Java
http://jblas.org
BSD 3-Clause "New" or "Revised" License
590 stars 149 forks source link

internal representation of DoubleMatrix fundamentally contradict with documentation #63

Closed boscogh closed 9 years ago

boscogh commented 9 years ago

from the documentation I expected that internally DoubleMatrix represented as single array of row after row, i.e. row0:column0, column2..column5; row1..., row2... etc

follow this logic, if I need to took DoubleMatrix[row1][column4], it would be length of 1 row + 4, i.e. "rowIndex * columns + columnIndex" "1 * 5+ 4"

but after week of fighting, I decided to look into code of DoubleMatrix.index method. /* Get index of an element / public int index(int rowIndex, int columnIndex) { return rowIndex + rows * columnIndex; }

it is completely opposite! so the internal representation of DoubleMatrix is NOT row after row. it is column, after column, because of this index method.

I am really surprised to find such fundamental difference. it took me more that 3 days of debug and test to understand that it was not my fault, because of my belief in documentation.

mikiobraun commented 9 years ago

Hi Boscogh,

sorry for the confusion. The memory layout is dictated by the use of the LAPACK and BLAS libraries which prefer this kind of layout called column major layout.

In order to fix this, can you point out where the documentation is misleading or false?

-M

boscogh commented 9 years ago

thanks for comment. yesterday I was so surprised that even started to doubt in DoubleMatrix.mmul operation, but of course it works. I'm sorry for some dramatization earlier. the documentation I mentioned is http://jblas.org/javadoc/org/jblas/DoubleMatrix.html#data there are also some sort of same statement in constructors http://jblas.org/javadoc/org/jblas/DoubleMatrix.html#DoubleMatrix(int, int, double...) http://jblas.org/javadoc/org/jblas/DoubleMatrix.html#DoubleMatrix(double[][]) and have a look into issue #60 - it is about another contructor http://jblas.org/javadoc/org/jblas/DoubleMatrix.html#DoubleMatrix(int) I understand you did the trick of transposing matrix in each contructor from double[][] to column ordered matrix, but this could be done by calling code, since it is anyway did some job on preparing data. in this case filling the 2D data-matrix would be much easier, and less time consuming, it would be just a set of System.arrayCopy operations.

That I was trying to achieve is to traverse DoubleMatrix in incremental order, i.e. DoubleMatrix.data[index++], and execute several computations at once, (back propagation of error algo) saving some time on addressing the memory. I read somewhere, that CPU will much faster operates with array if sequential access is used, rather then some complex indexing, because CPU always load a bunch of data to the cache not just single requested array item, so next time it access a nearby element of the array it is right there in the cache.

mikiobraun commented 9 years ago

Oh yeah, it almost looks like I consistently mixed up row and column... that's very bad.

I will need to fix that!

-M

boscogh commented 9 years ago

mikiobraun, hopefully you will have some time to deal with documentation in future. you know, I was trying to compare a performance of you lib, expecially in dgemv vs pure java and with JNI+MKL and JNA+MKL your library beat them all! great job! thank you for your work. I just realized you started almost 7 years ago, so it shouldn't now takes too much of your attention.

my "test" program internally performs a lot of dgemv calculations (I think >150000), for 1 single run with jblas it takes 9.769 seconds to complete, JNI + MKL (CBLAS) takes 10.552 seconds JNA + MKL (CBLAS) it is 11.211 seconds pure Java (linear incremental indexing) it takes 12.585 seconds it is the lowest time from several runs.

mikiobraun commented 9 years ago

Should be fixed now.