172 vector function

[DWiechert]

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

[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'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]

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!