Hi @SzechuanSage thanks for implementing these functions :)

What advantage do they give over http://mathjs.org/docs/reference/classes/sparsematrix.html#SparseMatrix+subset?

I think we can safely ignore the fact that codecov is giving this pull request a red cross

Refs: #1144 :)

I had a look at subset(). It requires the use of an Index object (and possibly a Range object). Maybe this would be acceptable for the poster of #1144. In any case, the row() and column() functions take a single parameter (a number) and they are quite short and self-contained.

Hi @SzechuanSage thanks for implementing these functions :)

What advantage do they give over http://mathjs.org/docs/reference/classes/sparsematrix.html#SparseMatrix+subset?

These are two utility functions, which make it easy to do something that is very easy in the expression parser (A[:,1] and A[1,:]) but quite verbose in plain JavaScript. Does that make sense to you @harrysarson?

@SzechuanSage your PR looks very neat, thanks!

Some feedback:

1. How would it work out if we let .column() and .row() simply call Sparse.subset() method internally? That may save quite some code, though it could cost a bit of performance (not sure). What do you think?
2. Thanks for working out unit tests. Can you also add some unit tests for invalid input (index out of range, matrix being 2D, index that is not an integer number).
3. I'm looking forward to see these methods implemented for DenseMatrix too :)

@SzechuanSage and @harrysarson one more thought: currently mathjs uses functions wherever possible, maybe we should also implement row and column as functions instead of class methods?

Could we have both - a function and a method?

How is A[:,1] implemented in the expression parser? It might make sense to use the new row and column functions?

So if row was a function, it would take a Matrix as the first parameter and a row index as the second parameter? What advantage does this have over keeping row as a Matrix method?

What advantage does this have over keeping row as a Matrix method?

I'm having two reasons in mind why this may make sense:

• So far, the whole design of the library has been to have data and (polymorphic) functions, so it would keep the library consistent.
• I'm currently working on making mathjs modular so you can use tree-shaking, i.e. bundle only the parts of the library that you use. A downside of a big class with methods is that you have to take it as a whole, you can't tree-shake them. For example if I only want to do +-/* with big numbers, I still have to bundle the whole Decimal.js library with all 40 or so methods. If these methods where implemented as static functions instead, I could pick just what I need.

How is A[:,1] implemented in the expression parser? It might make sense to use the new row and column functions?

I was thinking the other way around: row and column are eye-candy for doing A[:,1] (which uses subset), so we could simply have them call subset internally.

As for offering both a method and a function: we have that right now in some cases, like with diag which needs internals of matrices. I think though in general it's better to offer just one consistent way to do things. I would love to move the matrix and unit methods that we have right now out of the classes since they grow huge.

@josdejong this is your call. I am happy to add this functionality to \src\function\matrix. And if you like, remove it from \src\type\matrix. The unit tests can be replicated (or moved if row and column will not be methods any more). And I am happy to updated the documentation too.

Thanks @SzechuanSage, yes I prefer having functions instead of methods 👍

Thanks for your updates @SzechuanSage , looks really good. Thanks for taking care of unit tests and docs too!

Two things are left I think:

1. Can you undo the changes you made in DenseMatrix.test.js? These tests where really meant to test creating a DenseMatrix from a SparseMatrix.
2. I realize that we should also create transform functions, row.transform.js and column.transform.js, in order to make the behavior inside the expression parser one-based instead of zero-based. Similar to for example index.transform.js and concat.transform.js. If you want I can take care of that.

The only thing I would like confirmed is whether or not I have done the correct thing in the transform functions. Please let me know if any further changes are required.

Thanks @SzechuanSage! I will check it out coming weekend.

The transforms look good @SzechuanSage 👍

Two more things:

1. In both row.test.js and column.test.js, it turns out most of the tests are not actually executed: a nested it(...) inside another it(...) is not executed by mocha it seems. Can you flatten the tests? (I figured this out thanks to the coverage tests which claim that the actual logic in _row and _column is never executed.)
2. Can you write a unit test using both row and column in the expression parser (file parse.test.js), then we know for sure whether the transforms indeed work with one based indices.

Nice job Chris, thanks again 👍 I've merged the new functions, expect to do a release coming weekend.

I am happy I was able to help. Doing this taught me much.

The new functions row and column are now available in v5.9.0.

@SzechuanSage sorry for the huge delay, I mistakenly thought the functions where already released.