Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added some iterating methods to Matrix (issue #57) #79

Merged
merged 18 commits into from Aug 10, 2013

Conversation

@SamoylovMD
Copy link
Contributor

@SamoylovMD SamoylovMD commented Aug 7, 2013

Added: each, eachInRow, eachInColumn methods;
Renamed each method to eachNonZero.

@vkostyukov
Copy link
Owner

@vkostyukov vkostyukov commented Aug 8, 2013

Test are failed, since there is a usage if each in MatrixMarketStream. So, we need to change it to new eachNonZero call.

@@ -772,6 +803,33 @@ public void each(MatrixProcedure procedure) {
}

@Override
public void eachInRow(MatrixProcedure procedure, int i) {
Vector vector = this.getRow(i);

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

This is not a good idea. The getRow is extreamally slow method. We need to iterate in straightforward way. Like:

for (int j = 0; j < columns; j++) {
  procedure.apply(i, j, get(i, j));
}
@@ -772,6 +803,33 @@ public void each(MatrixProcedure procedure) {
}

@Override
public void eachInRow(MatrixProcedure procedure, int i) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

I would prefer to have signature like:

void eachInRow(int i, MatrixProcedure procedure);
}

@Override
public void eachInColumn(MatrixProcedure procedure, int j) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

I would prefer to have signature like:

void eachInColumn(int j, MatrixProcedure procedure);

@Override
public void eachInColumn(MatrixProcedure procedure, int j) {
Vector vector = this.getColumn(j);

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

And the same issue with getColumn(). Use straightforward iteration.

public void eachNonZero(MatrixProcedure procedure) {
for (int i = 0; i < rows; i++) {
for (int j = 0; j < columns; j++) {
if (Math.abs(get(i,j))>Matrices.EPS) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

That's fine. Just add a bit spaces. Like:

(Math.abs(get(i, j)) > Matrices.EPS)

This comment has been minimized.

@vkostyukov

vkostyukov Aug 9, 2013
Owner

I still have a concern about spaces.

@@ -408,6 +418,21 @@ public void each(MatrixProcedure procedure) {
}

@Override
public void eachInRow(MatrixProcedure procedure, int i) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

Swap arguments:

eachInRow(int i, MatrixProcedure procedure);
}

@Override
public void eachInColumn(MatrixProcedure procedure, int j) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

Swap arguments:

eachInColumn(int j, MatrixProcedure procedure);
* @param procedure
* @param i
*/
void eachInRow(MatrixProcedure procedure, int i);

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

Issue with arguments.

* @param procedure
* @param j
*/
void eachInColumn(MatrixProcedure procedure, int j);

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

Issue with arguments order.

@@ -286,6 +286,52 @@ public void each(MatrixProcedure procedure) {
}

@Override
public void each(MatrixProcedure procedure) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

We don't need this. There is each method in AbstractMatrix class that has the same logic. And we can't do it better than there.

}

@Override
public void eachInColumn(MatrixProcedure procedure, int j) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

For now, it would be enough to have eachInRow() in general implementation for sparse matrices. I mean, AbstractMatrix.eachInRow() should be enough. So, we don't need this concreet method for CCS.

}

@Override
public void eachInRow(MatrixProcedure procedure, int i) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

The same. I don't see the reason having this method in CCS. It would be enough to have in general.

@@ -360,6 +360,52 @@ public void each(MatrixProcedure procedure) {
}

@Override
public void each(MatrixProcedure procedure) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

We don't need a separate implementation of each method for CRS matrix. We already have it in AbstractMatrix.

This comment has been minimized.

@SamoylovMD

SamoylovMD Aug 9, 2013
Author Contributor

I thought that implementation of getting an element in CRS and CCS matrices works longer than O(1). So, my implementation works for O(rows*columns) which is faster than method inherited from AbstractMatrix.

This comment has been minimized.

@vkostyukov

vkostyukov Aug 9, 2013
Owner

Right. The get/set works in O(log rows) for CCS and in O(log columns) for CRS. Probably, you're right. We might need each methods for CRS/CCS.

}

@Override
public void eachInRow(MatrixProcedure procedure, int i) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

We don't need this. We do have such method in AbstractMatrix.

}

@Override
public void eachInColumn(MatrixProcedure procedure, int j) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 8, 2013
Owner

For now, it would be enough to have this in AbstractMatrix.

@vkostyukov
Copy link
Owner

@vkostyukov vkostyukov commented Aug 8, 2013

Hi Maxim @SamoylovMD,

Please also add eachNonZeroInRow() and eachNonZeroInColumn() implementations. And we can provide more efficient implementation of eachNonZeroInRow() for CRS matrix and eachNonZeroInColumn for CCS matrix. For other cases, it would be enough to have general implenetation in AbstractMatrix class.

Please, also follow my coments to improve the code.

@@ -28,8 +28,10 @@
import java.util.Random;

import org.la4j.decomposition.MatrixDecompositor;
import org.la4j.factory.Basic2DFactory;

This comment has been minimized.

@vkostyukov

vkostyukov Aug 9, 2013
Owner

Why do we need this?

import org.la4j.factory.Factory;
import org.la4j.inversion.MatrixInvertor;
import org.la4j.matrix.dense.Basic2DMatrix;

This comment has been minimized.

@vkostyukov

vkostyukov Aug 9, 2013
Owner

And this?


@Override
public void eachNonZeroInRow(int i, MatrixProcedure procedure) {
eachInRow(i,procedure);

This comment has been minimized.

@vkostyukov

vkostyukov Aug 9, 2013
Owner

I don't think this is correct. We need a separate implementation for non-zero elements here.

This comment has been minimized.

@SamoylovMD

SamoylovMD Aug 9, 2013
Author Contributor

Ofc, my error. Will rewrite in few seconds.

This comment has been minimized.

@SamoylovMD

SamoylovMD Aug 9, 2013
Author Contributor

Fixed in new commit.

for (int i = 0; i < rows; i++) {
for (int j = 0; j < columns; j++) {
if (Math.abs(get(i,j)) > Matrices.EPS) {
procedure.apply(i,j,get(i,j));

This comment has been minimized.

@vkostyukov

vkostyukov Aug 10, 2013
Owner

I would prefere to have spaces here. Like

procedure.apply(i, j, get(i, j));
@Override
public void eachNonZeroInRow(int i, MatrixProcedure procedure) {
for (int j = 0; j < columns; j++) {
if (get(i, j) > Matrices.EPS) procedure.apply(i,j,get(i,j));

This comment has been minimized.

@vkostyukov

vkostyukov Aug 10, 2013
Owner

It seems Math.abs() is missed. What about negative values?
And also please, add {} arround the procedure.apply(...)

@Override
public void eachNonZeroInColumn(int j, MatrixProcedure procedure) {
for (int i = 0; i < rows; i++) {
if (get(i, j) > Matrices.EPS) procedure.apply(i,j,get(i,j));

This comment has been minimized.

@vkostyukov

vkostyukov Aug 10, 2013
Owner

Again: missed Math.abs() and {} arround procedure.appy(...).


@Override
public void eachInColumn(int j,MatrixProcedure procedure) {
self.eachInColumn(j,procedure);

This comment has been minimized.

@vkostyukov

vkostyukov Aug 10, 2013
Owner

Need space: j, procedure.


@Override
public void eachNonZeroInColumn(int j, MatrixProcedure procedure) {
self.eachNonZeroInColumn(j,procedure);

This comment has been minimized.

@vkostyukov

vkostyukov Aug 10, 2013
Owner

Need space: j, procedure.

@@ -286,6 +286,44 @@ public void each(MatrixProcedure procedure) {
}

@Override
public void each(MatrixProcedure procedure) {
int l = 0;

This comment has been minimized.

@vkostyukov

vkostyukov Aug 10, 2013
Owner

Why didn't you call it k instead of l. I mean, in the eachInColumn it's called k.

}

@Override
public void eachNonZeroInColumn(int j, MatrixProcedure procedure) {

This comment has been minimized.

@vkostyukov

vkostyukov Aug 10, 2013
Owner

What about having a simple for loop here. Like:

for (int i = columnPointers[k]; i < columnPointers[j + 1]; i++) {
    procedure.apply(rowIndices[i], j, values[i]);
}

There is something wrong with your code. First, i = rowIndicies[k]. And then you're getting row by rowIndicies[i]. This is strange. I guarantee it will fail tests.

This comment has been minimized.

@SamoylovMD

SamoylovMD Aug 10, 2013
Author Contributor

But this loop

for (int i = columnPointers[k]; i < columnPointers[j + 1]; i++) {
    procedure.apply(rowIndices[i], j, values[i]);
}

will apply procedure to non-zero values column only, won't it? Your implementation looks like mine in CCSMatrix.eachNonZeroInColumn

@@ -360,6 +360,44 @@ public void each(MatrixProcedure procedure) {
}

@Override
public void each(MatrixProcedure procedure) {
int l = 0;

This comment has been minimized.

@vkostyukov

vkostyukov Aug 10, 2013
Owner

Let's rename it to k.


@Override
public void eachNonZeroInRow(int i, MatrixProcedure procedure) {
int k = rowPointers[i], j = columnIndices[k];

This comment has been minimized.

@vkostyukov

vkostyukov Aug 10, 2013
Owner

Again strange loop. We need something like this:

for (int j = rowPointers[i]; j < rowPointers[j + 1]; j++) {
    procedure.apply(i, columnIndices[j], values[j]);
}
SamoylovMD added 5 commits Aug 10, 2013
@vkostyukov
Copy link
Owner

@vkostyukov vkostyukov commented Aug 10, 2013

So, I believe the best way to make sure that all implementations are correct - is to write tests. Could you please write simple tess for all the methods (esp. eachIn.. and eachNonZeroIn...) in AbstractMatrixTest class.

Here is some ideas how to test it. We can write simple matrix procedure that dumps row into double array and then check it. Like this:

public static class RowDumper implements MatrixProcedure {
  double data[];
  public Dumper(int n) {
    data = new double[n];
  }

  public apply(int i, int j, double) {
    data[j] = value;
  }
}

Matrix a = new CRSMatrix(...);
RowDumper d = new RowDumper(a.columns());
a.eachInRow(0, d);
// check whether the d.data is correct
a.eachNonZeroInRow(1, d);
// check whether the d.data is correct
@SamoylovMD
Copy link
Contributor Author

@SamoylovMD SamoylovMD commented Aug 10, 2013

EachInRow works correct, EachNonZeroInRow needs correction.

SamoylovMD added 2 commits Aug 10, 2013
Fixed bug with eachNonZeroInRow()

@Override
public void eachNonZeroInRow(int i, MatrixProcedure procedure) {
int k = rowPointers[i], j = k;

This comment has been minimized.

@vkostyukov

vkostyukov Aug 10, 2013
Owner

So, now it looks correct, but I still don't see the reason of having this k variable. For what it is?
And I would prefer having a for loop like (equivalent to your while loop but much clear):

for (int j = rowPointers[i]; j < rowPointers[i + 1]; j++) {
  procedure.apply(i, columnIndices[j], values[j]);
}

@Override
public void eachNonZeroInColumn(int j, MatrixProcedure procedure) {
int k = columnPointers[j], i = k;

This comment has been minimized.

@vkostyukov

vkostyukov Aug 10, 2013
Owner

So, now it looks correct, but I still don't see the reason of having this k variable. For what it is?
And I would prefer having a for loop like (equivalent to your while loop but much clear):

for (int i = columnPointers[j]; i < columnPointers[j + 1]; i++) {
  procedure.apply(rowIndices[i], j, values[i]);
}
SamoylovMD added 3 commits Aug 10, 2013
Refactored eachNonZeroInColumn method
Fixed error
Refactored eachNonZeroInRow method
@vkostyukov
Copy link
Owner

@vkostyukov vkostyukov commented Aug 10, 2013

Thank you Maxim @SamoylovMD!

vkostyukov added a commit that referenced this pull request Aug 10, 2013
Added some iterating methods to Matrix (fixes #57)
@vkostyukov vkostyukov merged commit c7b3843 into vkostyukov:master Aug 10, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.