echNonZeroInRow() and eachNonZeroInColumn should take VectorFunction instead

[vkostyukov]

Other variants should be deprecated in this milestone.

[vkostyukov]

Also the same refactoring should be done for accumulators.

[vkostyukov]

These methods should be marked deprecated in this release. And should be removed at the next release 0.6.0.

[DWiechert]

Are you saying that the following should be deprecated?

• eachNonZero
• eachNonZeroInRow
• eachNonZeroInColumn
• foldNonZero
• foldNonZeroInRow
• foldNonZeroInColumn
• foldNonZeroInColumns
• foldNonZeroInRows

And instead of taking in a MatrixProcedure/MatrixAccumulator they should take in a VectorFunction? Why should it be a VectorFunction and not a MatrixFunction?

[vkostyukov]

Not all of those. Only InRow and InColumn. I doesn't make sense to use MatrixFunction there since it takes both of the indices, but one of those indices will be fixed when iterating through the row/column. There is nothing to do with eachNonZero and foldNonZero: there are iterating the entire matrix and take matrix function.

[DWiechert]

I'm trying to understand how a MatrixProcedure will change to a VectorFunction and wanted to know if this is correct:

Current method:

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

New method:

@Override
public void eachNonZeroInRow(int i, VectorFunction function) {
    // FIXME
    for (int j = 0; j < columns; j++) {
        if (Math.abs(get(i, j)) > Matrices.EPS) {
            function.evaluate(j, get(i, j));
        }
    }
}

[vkostyukov]

Wait, where did you find this eachNonZeroInRow method? I'm surprised out the implementation.

Looks like the ticket description wasn't clear. It's mu bad - I usually leave tickets to myself, so I keep some context in my head. It's time to change it.

So, we don't need to replace procedure witch function (like in your example), but we should make all the row or column iterating methods take vector procedure/function instead, since we iterate through the row/column and we have one of the dimension fixed. Simply speaking:

void eachNonZeroInRow(int i, MatrixProcedure) // wrong
void eachNonZeroInRow(int i, VectorProcedure) // ok, since i-th row is a vector
void foldNonZeroInRow(int i, MatrixFunction) // wrong
void foldNonZeroInRow(int i, VectorFunction) // ok, since i-th row is a vector

What about implementation of eachNonZeroInRow?

[vkostyukov]

Oh, I see. It's in AbstractCompressed matrix. Basically, we have to get rid of those slow implementations. But it's not that easy.

[DWiechert]

Ok, maybe I'm not fully understanding this issue then. Is there more to just swapping MatrixProcedure to VectorFunction?

Also, I thought there were 4 methods getting deprecated/added:

Deprecated

void eachNonZeroInRow(int i, MatrixProcedure procedure);
void eachNonZeroInColumn(int j, MatrixProcedure procedure);
double foldNonZeroInRow(int i, MatrixAccumulator accumulator);
double foldNonZeroInColumn(int j, MatrixAccumulator accumulator);

Added:

void eachNonZeroInRow(int i, VectorFunction function);
void eachNonZeroInColumn(int j, VectorFunction function);
double foldNonZeroInRow(int i, VectorFunction function);
double foldNonZeroInColumn(int j, VectorFunction function);

[DWiechert]

Oh, I see it now. eachNonZeroInRow/Column should take in a VectorProcedure and foldNonZeroInRow/Column takes in the VectorFunction.

[vkostyukov]

Right. BTW, we can just remove these two guys:

double foldNonZeroInRow(int i, MatrixAccumulator accumulator);
double foldNonZeroInColumn(int j, MatrixAccumulator accumulator);

Since they were added this release.

[DWiechert]

This has been fixed by PR #233.

[DWiechert]

@vkostyukov, since it seems this issue can be closed, is there a next issue you would like me to look into?

[vkostyukov]

Hi @DWiechert! How about this one #227? It might be interesting for you.