172 vector function

[DWiechert]

Preliminary code review of new functions. Tests will be added once I know the implementation is correct.

[vkostyukov]

Please, remove FIXME.

[DWiechert]

Of course, I just put these in to remind myself what I need to test. They will be removed.

[vkostyukov]

Thanks @DWiechert!

So, the rule is pretty simple:

each methods take XProcedure
fold methods take XAccumulator
update methods take XFunction, but we need to change them, I believe,

[vkostyukov]

We also need to refactor

• Matrix.eachInRow(int, MatrixProcedure) -> Matrix.eachInRow(int, VectorProcedure)
• Matrix.eachInColumn(int, MatrixProcedure) -> Matrix.eachInColumn(int, VectorProcedure)
• Matrix.foldRow(int, MatrixAccumulator) -> Matrix.foldRow(int, VectorAccumulator)
• Matrix.foldColumn(int, MatrixAccumulator) -> Matrix.foldColumn(int, VectorAccumulator)
• Matrix.foldColumns(MatrixAccumulator) -> Matrix.foldColumns(VectorAccumulator)

[DWiechert]

Why does Matrix.foldColumns(MatrixAccumulator) need to change? Or, if it does need to change, shouldn't other similar methods also change?

• Matrix.foldRows(MatrixAccumulator
• SparseMatrix.foldNonZeroInRows(MatrixAccumulator)
• SparseMatrix.foldNonZeroInColumns(MatrixAccumulator)

[vkostyukov]

Right. We have to change all of them. The problem is the same. foldRows does exactly what foldRow(int) does but accumulates every folded value into a result vector. Let's say we have 5x5 matrix. foldRows() on that matrix will return a vector of length 5. Every i-th value in that vector is a result of foldRow(i) function.

[DWiechert]

So, is EVERY method in SparseMatrix and Matrix that takes in 1 of those 3 (MatrixProcedure, MatrixFunction, MatrixAccumulator) getting changed to their new Vector equivalents?

[vkostyukov]

Not really, only those methods that works with columns/rows (those whose name is endding with InRow or InColumn).

Look, there is method w/o that postfix, like fold that takes MatrixAccumulator. This method applies given accumulator to every cell in a matrix. There are methods like foldInColumn that takes MatrixAccumulator (for now, but this is wrong). These methods apply the give accumulator to every cell in a given row/column of the matrix. So we are working with just vectors, since row/column is a vector.

Matrix a =???
a.foldInRow(i, Vectors.asSumAccumulator(0.0)); // should be equivalent to
a.getRow(i).fold(Vectors.asSumAccumulator(0.0)); // a.getRow(i) returns Vector, then we cal fold on Vector

[DWiechert]

@vkostyukov, I think I'm fully understanding this issue now and have made some changes. When you get a chance, can you look over the methods I deprecated and the new versions added in Matrix and SparseMatrix to verify those are the interface changes you wanted? Once I know I've made all of the correct interface changes I'll get to work on the implementations and tests.

[vkostyukov]

@DWiechert this looks good! I've totally forgot about transformRow/transformColumn functions. Thanks for handling them!

[DWiechert]

Thanks for taking a quick look at the interface changes and verifying they're ok. Once I understood the changes that needed to be done it was pretty trivial. I'll get started on the implementation/testing later today and will update this issue when it's ready for the final review.

[DWiechert]

@vkostyukov, I'm unsure about the implementation of this method and updateColumn. The reason why I'm confused is because for the one that takes in a MatrixFunction, it updates a single point, not a specific row/column. I was wondering if you could explain how you think this updateRow/Column should work. Or if these are not to be updated, let me know.

[vkostyukov]

I see what you mean. We have a function updateRow that should take a VectorFunction, but it calls update that takes MatrixFunction. You need some sort of converters between vector function and matrix function. In my head it looks like following:

class RowVectorToMatrixFunction(VectorFunction underlying) implements MatrixFunction {
  double evaluate(int i, int j, double value) {
     return underlying.evaluate(j, value);
  }
}

class ColumnVectorToMatrixFunction(VectorFunction underlying) implements MatrixFunction

@Override
public void updateRow(int i, VectorFunction function) {
    Matrix function underlying = new RowVectorToMatrixFunction(function);
    for (int j = 0; j < columns; j++) {
        update(i, j, underlying);
    }
}

We have two converters here RowVectorToMatrixFunction that only consider to work with row-vector (only care about j-index) and an opposite case ColumnVectorToMatrixFunction.

[DWiechert]

@vkostyukov, I've added the implementations to all of the new interface methods. I noticed there weren't a ton of original tests and I just copied the original ones and pointed to the new methods. Is that sufficient or would you like me to add tests for each individual method?

[vkostyukov]

Something wrong with indentation.

[vkostyukov]

Why fixme?

[DWiechert]

Whoops, looks like I missed removing this one once I did the implementation. I'll remove this.

[vkostyukov]

Thanks @DWiechert! This looks good. Could you please check the indentation and remove the FIXME notes?

[DWiechert]

@vkostyukov, I've made those code review changes about indentation and removing the FIXMEs.

[vkostyukov]

Thanks @DWiechert! This looks great!