Simplify getting and setting SimpleBase rows/columns

[JozsefKutas]

I've recently started using EJML, and the SimpleBase API seems a bit clunky when it comes to getting/setting rows/columns.

1. The SimpleBase#extractVector takes a boolean parameter which determines whether a row or column is extracted. I don't think theres any obvious reason why false should correspond to columns and true to rows, so it's easy to get the two confused.

2. SimpleBase#extractVector returns a matrix, while SimpleBase#setRow and SimpleBase#setColumn take a double array, so, for example, to set one row/column to another row/column requires something like:

matrix.setColumn(j, 0, matrix.extractVector(false, j).getDDRM().data);

vs. Apache Commons:

matrix.setColumn(j, matrix.getColumn(j));

To fix 1, maybe SimpleBase#extractVector could be supplemented with methods extractRow and extractColumn (or row and column, to be consistent with SimpleBase#rows and SimpleBase#columns). To fix 2, SimpleBase#setRow and SimpleBase#setColumn could be overloaded to allow for matrix row/column arguments.

Otherwise I'm enjoying using EJML, so thanks!

[lessthanoptimal]

Thanks for the suggestions! Yeah I think SimpleMatrix is in need of a refresh. I'll look into these suggestions. Feel free to post other ones too!

[lessthanoptimal]

Just letting you know I plan to work on this soon.

[JozsefKutas]

OK, thanks for the heads up. I had a few more thoughts after using EJML a bit more, but didn't get around to putting them in writing. I'm a bit bogged down with work atm, but should be able to respond to this issue and the other either tonight or tomorrow.

[lessthanoptimal]

I can relate to that.

[JozsefKutas]

Hi,

A few thoughts, in no particular order:

1. The SimpleMatrix documentation describes it as a wrapper around DMatrixRMaj when it can actually wrap other matrix types.

2. Method naming is sometimes a bit inconsistent: some methods use the get prefix (get, getNumElements, ...), some use extract (extractVector, ...), some use neither (rows, numRows, ...). This makes it harder to locate methods in a large API.

3. Complex matrix support is pretty limited, but maybe this is by design. There are no methods for adding/subtracting/multiplying/dividing by complex numbers, extracting the real/imaginary part, or taking the conjugate. It would also seem more natural for the complex entry setter to have signature SimpleMatrix#set(int row, int col, Complex_F64 complex) to match SimpleMatrix#get​(int row, int col, Complex_F64 output).

4. The concatenation methods are convenient when concatenating a few matrices, e.g. A.concatRows(B, C), but are unwieldy when concatenating a list of matrices. The lack of shape compatibility checks may sometimes be useful, but I'd usually rather an error were thrown. I ended up using static utility methods to check that the list is non-empty and that sizes are compatible, and if so, dispatch on the first element.

5. It would be useful to be able to sum over rows/columns. I can see this is supported in CommonOps_*, but it would be good if it could be done with SimpleMatrix.

6. The out-of-bounds error for cols/rows/extractVector/extractMatrix is not very clear. It references the argument names from CommonOps_*#extract, which are different to those from SimpleMatrix. The shape comprison is never useful for extractVector, and isn't always helpful for extractMatrix. Example:

SimpleMatrix matrix = new SimpleMatrix(2, 2);
System.out.println(matrix.extractVector(false, 3));
Exception in thread "main" org.ejml.MatrixDimensionException: srcX1 < srcX0 || srcX0 < 0 || srcX1 > src.numCols. ( 2x2 ) ( 2x1 )

7. The additions in #148 look useful, but filled and ones could allow an optional Class parameter (consistent with the constructors, diag and identity).

8. I'd find it useful to have an immutable version of SimpleMatrix, in a similar vein to Guava's immutable collections or the immutable collections introduced in Java 9/10 (created using List.of, List.copyOf, ...). I think this could be achieved by extending SimpleBase and throwing an UnsupportedOperationException in methods that could be used to mutate the matrix. If this is outside EJML's scope, I'll probably just end up doing it in my own code - at the moment I'm making do with taking defensive copies.

Some of these things it may not be possible/practical to change. If you want any help making any changes, I'd be happy to contribute PRs.

[lessthanoptimal]

@JozsefKutas could you maybe take a crack at improving concatenation as you described above and create a new issue for it for it?

[lessthanoptimal]

https://github.com/lessthanoptimal/ejml/pull/170 is the PR which adds the original request here for row/column functions. It's dependent on the complex PR, which is why there are so many changes.

[JozsefKutas]

Looks good! I should be able to create an issue and PR for concatenation on Saturday.

[lessthanoptimal]

Going to close this ticket, but it would be helpful if you created new PRs for specific items in the wish list above that haven't been finished yet. @JozsefKutas

[JozsefKutas]

Going to close this ticket, but it would be helpful if you created new PRs for specific items in the wish list above that haven't been finished yet.

Will do, and sorry about the delay in contributing. I've got a major deadline at the end of this month, so that's been taking up most of my time. I'll see what I can manage in the meantime, but it might not be much for the next week or two.

[lessthanoptimal]

@JozsefKutas any chance you could create a new PR for any suggestions that were missed in the last update?

[JozsefKutas]

Yup, I should be able to do a PR this weekend or sometime next week. I've had it on my todo list, but I haven't had much free time recently.

[JozsefKutas]

Sorry for the delay here. I think you've actually covered the main points, but I'll open a couple of PRs. I've converted the suggestions above into the task list below:

• [x] Update references to the internal matrix of SimpleMatrix being a DMatrixRMaj.
• [x] Update method naming to improve consistency (this involves breaking changes, so I can make a couple of suggestions for you to take or leave).
• [x] Improve complex matrix support.
• [ ] Add concatenation static factory methods to SimpleMatrix.
• [ ] Add methods to sum/aggregate over rows/columns.
• [ ] Improve out-of-bounds error messages.
• [x] Add an optional Class parameter to filled and ones.
• [x] Add an immutable version of SimpleMatrix.