vkostyukov / la4j

Linear Algebra for Java
http://la4j.org
Apache License 2.0
373 stars 109 forks source link

Setting elements to zero in CRSMatrix #253

Closed rabbanyk closed 9 years ago

rabbanyk commented 9 years ago

Hi there,

I was trying to set some elements in a Sparse Matrix (CRSMatrix) to zero, using nonZeroIterator, or apply procedure, and it seems to mess with the matrix and cause non valid results for further operations, in my case subtract. To be more specific, I'm extending the CRS with the following method:

public void zeroDiagonalsInPlace() {
        MatrixIterator it = nonZeroIterator();
        while (it.hasNext()) {
            double x= it.next();
            if (it.rowIndex() == it.columnIndex()) 
                        it.set(0);
        }
    }
or 
    MatrixProcedure procedure = new MatrixProcedure() {
            @Override
            public void apply(int i, int j, double value) {
                if (i==j)
                    set(i,j,0);
            }
        };
         this.eachNonZero(procedure );

The method seems to do the job but then I am subtracting another matrix from this resulted matrix, and the results are wrong. See example bellow, A = 1.000 0.000 0.000 1.000 1.000 0.000 1.000 0.000 0.000 0.000 0.000 0.000 1.000 0.000 0.000 1.000 0.000 0.000 1.000 1.000 1.000 0.000 0.000 1.000 1.000

B = 1.000 0.000 0.000 1.000 1.000 0.000 1.000 1.000 1.000 0.000 0.000 1.000 2.000 2.000 1.000 1.000 1.000 2.000 3.000 2.000 1.000 0.000 1.000 2.000 2.000

A.zeroDiagonalsInPlace()
B.zeroDiagonalsInPlace()

A = 0.000 0.000 0.000 1.000 1.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 1.000 0.000 0.000 0.000 1.000 1.000 0.000 0.000 1.000 0.000 B = 0.000 0.000 0.000 1.000 1.000 0.000 0.000 1.000 1.000 0.000 0.000 1.000 0.000 2.000 1.000 1.000 1.000 2.000 0.000 2.000 1.000 0.000 1.000 2.000 0.000

A.subtract(B)

0.000 0.000 0.000 0.000 0.000 1.000 0.000 -1.000 1.000 1.000 0.000 -1.000 0.000 -2.000 -1.000 -1.000 -1.000 -2.000 0.000 -2.000 -1.000 0.000 -1.000 -2.000 0.000

vkostyukov commented 9 years ago

Hi @rabbanyk! Thanks for the report. What happens if you subtract B from B w/o calling zeroDiagonalsInPlace(). Does it work?

rabbanyk commented 9 years ago

The result for A-B before setting diagonal elements to zero is correct, i.e. 0.000 0.000 0.000 0.000 0.000 0.000 0.000 -1.000 -1.000 0.000 0.000 -1.000 -1.000 -2.000 -1.000 0.000 -1.000 -2.000 -2.000 -1.000 0.000 0.000 -1.000 -1.000 -1.000

vkostyukov commented 9 years ago

Seemed to be a bug, I will have a look on it today. Did you try the same scenario with CCSMatrix?

rabbanyk commented 9 years ago

No, but I think the problem is with setting an element to zero. Same happens with

public void zeroDiagonalsInPlace() {
    for(int i =0; i<this.rows();i++)
            this.set(i, i, 0);
}
vkostyukov commented 9 years ago

Really interesting. I will try to troubleshot it today. If you find a root-cause - just let me know.

vkostyukov commented 9 years ago

BTW, can you try to call zeroDiagonalsInPlace and then print matrix using iterator? For example:

MatrixIterator it = a.nonZeroIterator();
while (it.hasNext()) {
  double x = it.next();
  int i = it.rowIndex();
  int j = it.columnIndex();
  System.out.println("A[" + i + "," + j + "] = " + x);
}
rabbanyk commented 9 years ago

for A it gives: 0.000 0.000 0.000 1.000 1.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 1.000 0.000 0.000 0.000 1.000 1.000 0.000 0.000 1.000 0.000

[0,3] = 1.0 [0,4] = 1.0 [1,0] = 1.0 [1,4] = 1.0 [1,0] = 1.0 [1,3] = 1.0

And for B: 0.000 0.000 0.000 1.000 1.000 0.000 0.000 1.000 1.000 0.000 0.000 1.000 0.000 2.000 1.000 1.000 1.000 2.000 0.000 2.000 1.000 0.000 1.000 2.000 0.000

[0,3] = 1.0 [0,4] = 1.0 [1,2] = 1.0 [1,3] = 1.0 [2,1] = 1.0 [2,3] = 2.0 [2,4] = 1.0 [3,0] = 1.0 [3,1] = 1.0 [3,2] = 2.0 [3,4] = 2.0 [4,0] = 1.0 [4,2] = 1.0 [4,3] = 2.0

vkostyukov commented 9 years ago

Which is wrong, right? The issue with set(i, j, 0.0) method. Thanks for the report. I will fix it!

rabbanyk commented 9 years ago

Yes, and Thanks.

rabbanyk commented 9 years ago

I think the problem is with having a row with all zero elements. See for example the following which is not using the set method:

        double [][] a = {{1,0,1,0,0},{0,0,0,0,0},{0,0,0,0,0},{1,0,0,0,0},{1,0,0,1,0}};
        CRSMatrix m = new CRSMatrix(a);
        System.err.println(m);

        MatrixIterator it = m.nonZeroIterator();
        while (it.hasNext()) {
              double x = it.next();
              int i = it.rowIndex();
              int j = it.columnIndex();
              System.err.println("[" + i + "," + j + "] = " + x);
            }
        return;

1.000 0.000 1.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 0.000 1.000 0.000 0.000 0.000 0.000 1.000 0.000 0.000 1.000 0.000

[0,0] = 1.0 [0,2] = 1.0 [1,0] = 1.0 [1,0] = 1.0 [1,3] = 1.0

vkostyukov commented 9 years ago

Right. I've reproduced it using the similar test. The issue is nonZeroIterator.

vkostyukov commented 9 years ago

Here come the fix! Thanks @rabbanyk! Grab the sources from repo and run mvn install and depend you source code on version 0.5.5. Let me know if you face the issue with javadoc generation.