LCSL / GURLS

GURLS: a Least Squares Library for Supervised Learning
http://lcsl.mit.edu/#/downloads/gurls
63 stars 37 forks source link

Possible issue with function getRow in gmath.h #12

Closed raffaello-camoriano closed 10 years ago

raffaello-camoriano commented 10 years ago

Function getRow (https://github.com/LCSL/GURLS/blob/master/gurls%2B%2B/include/gurls%2B%2B/gmath.h#L1448) (called, among others, by the operator[] of class gMat2D) seems not to return an array containing cols elements of row _rowindex of matrix M. On the contrary, it returns an array with cols elements picked along the _rowindex-th column of matrix M. If I am not missing anything relevant, this is not consistent with the function description: "Generates a vector containing a copy of a row of an input matrix"

template<typename T>
void getRow(const T* M, const int rows, const int cols, const int row_index , T* row)
{
    copy(row, M+row_index, cols, 1, rows);
}

Modifying the copy function call in the following way seems to solve the problem.

template<typename T>
void getRow(const T* M, const int rows, const int cols, const int row_index , T* row)
{
    copy(row, M + row_index * cols, cols, 1, 1);
}

The piece of code I used for debug is the following. If needed, I can provide all the source code.

// Explicit copy
for (int i = 0; i<nte; i++)
{
[...]

for (int k = 0; k<t; k++)
{
    if(verbose) std::cout << k+1 << "-th element of resptr->getData(): " << *(resptr->getData() + k) << std::endl;

    *(yte_pred.getData() + k +  i * t) = *(resptr->getData() + k);
    if(verbose) std::cout << k+1 << "-th element of yte_pred.getData(): " << *(yte_pred.getData() + k +  i * t) << std::endl;
}
if(verbose) std::cout << std::endl;
if(verbose) std::cout << "Result stored in matrix yte_pred" << std::endl;  
if(verbose) std::cout << "Stored data:" << std::endl << yte_pred[i] << std::endl;  

[...]
}
andreaschiappacasse commented 10 years ago

It seems to me that it's working as intended: Internal buffers of gMat objects are stored in Column-major order, so it should be correct that the first index to copy in the buffer is _M+rowindex, and that the increment is rows.

Opening a test data matrix and printing the i th rows with operator[] gives the expected output to me.

Looking at your code, even if I don't know the internal geometry of resptr , it is possible that you are copying it into _ytepred in the wrong way, causing your result to be incorrect.

If resptr contains the i th row, it should be copied into your output buffer this way into your cycle:

*(yte_pred.getData() + i + k*nte) = *(resptr->getData() + k);

If the above informations doesn't help you in solving your problem, please provide me the source code, so that i can look into it.

Andrea

raffaello-camoriano commented 10 years ago

Thank you, I was assuming the stored matrix to be in Row-major order.

My test now runs correctly:

---------------------------------------
Prediction rows: 1
Prediction cols: 6
Prediction:
[ -15.6105 16.8524 -18.1854 -0.199317 0.177367 0.209763 ]
---------------------------------------
t = 6
1-th element of resptr->getData(): -15.6105
1-th element of the updated row of yte_pred: -15.6105
2-th element of resptr->getData(): 16.8524
2-th element of the updated row of yte_pred: 16.8524
3-th element of resptr->getData(): -18.1854
3-th element of the updated row of yte_pred: -18.1854
4-th element of resptr->getData(): -0.199317
4-th element of the updated row of yte_pred: -0.199317
5-th element of resptr->getData(): 0.177367
5-th element of the updated row of yte_pred: 0.177367
6-th element of resptr->getData(): 0.209763
6-th element of the updated row of yte_pred: 0.209763

Result stored in matrix yte_pred
Stored data:
[-15.6105 16.8524 -18.1854 -0.199317 0.177367 0.209763]

I guess this issue can be closed.