Matrix creation and conversion methods

[cshaa]

This PR brings several quality-of-life improvements to matrices.

• DenseMatrix and SparseMatrix are now iterable, which fixes Make Matrix iterable (support for...of statement) #1184
• matrixFromFunction(size, fn), which fixes Feature Request: Array/Matrix From Function #2153
• matrixFromRows and matrixFromColumns
• mat.rows() and mat.columns() now return arrays of vectors, see Add methods for iteration of matrix rows and columns #2177
• matrix, DenseMatrix and SparseMatrix accept an iterable as data
• sparseFromDOK, sparseFromLIL, sparseFromCOO, sparseFromCSR, sparseFromCSC (wiki), which fixes (not) Constructing sparse matrices on your own? #1166
• SparseMatrix now has methods toDOK, toLIL, toCOO, toCSR, toCSC
• documentation of SparseMatrix is now less sparse, which fixes (not) Improve the docs for sparse matrices #1769

The strikethrough points will be addressed in a followup PR.

[cshaa]

The build fails because of this error:

ReferenceError: regeneratorRuntime is not defined

It is caused by the fact that I use a generator function but the Babel's generator polyfill is not properly loaded, see this thread on SO. @josdejong I think I'll need your help with this one.

[josdejong]

I think you can rewrite this to:

const expected = m.map((value, index) => ({ value, index }))

[josdejong]

O no, nevermind, that returns a DenseMatrix again

[josdejong]

wow good busy @m93a 😎

I personally prefer a single PR for a single new feature instead of one huge PR adressing lots of things: it makes it much harder to review (and easy to overlook something), keeps new functionality stuck in the pipeline for a longer time (putting pressure on you to everything asap). Is it ok for you to just handle the first three checkboxes in this PR, and address the other checkboxes in separate PR's?

Some feedbacks on your implementation:

1. The new util functions matrixFromRows and matrixFromColumns can be useful, thanks. Maybe you can also mention the functions column and row in the documentation under "See also"?

2. The function _creatArray and _createMatrix are almost exactly the same. I think it should be possible to remove _createMatrix, and replace

   return _createMatrix(arr)

   with something like:

   return matrix(_createArray(arr.map(item => item.toArray())))

3. The following code can be simplified:

   let s
   if (Array.isArray(vec)) {
     s = size(vec)
   } else {
     s = vec.size()
   }

   to:

   const s = size(vec)

4. Can you give some use cases for the new methods DenseMatrix.rows and DenseMatrix.columns? It is very close to the existing functions row and column. It introduces a new return type though which we discussed in the eigenvalues/vectors PR too: a nested array containig matrices. This may be useful methods but I want to be careful here since this construct may complicate matters. People will use the result of .rows() in other mathjs function like size(), but non of the mathjs functions understands this concept of an Array containing Matrices and will handle it expecting an Array containing Arrays, which probably gives weird/wrong/undefined behavior.

5. I'll have a look into the regeneratorRuntime error you're encountering.

[josdejong]

@m93a about the regeneratorRuntime error: I've added @babel/transform-runtime to the .babelrc, if you merge develop in your feature branch you should be good to go. See 64a52a5

[josdejong]

Hm, the @babel/transform-runtime configuration gave issues, see #2169. I've reverted this for now, but we should find a solution in order to be able to use iterators.

[cshaa]

1. Done!
2. Done!
3. Done!
4. The methods Matrix.columns and Matrix.rows should return an array (or an iterable) of the matrix's columns or rows, which could be used in for..of. I only added them to Matrix because it's already easy to iterate through arrays. However, now that you mention it, my proposal might be a little controversial and it might be better to leave it out of this PR and discuss it properly in a separate issue. Starting Add methods for iteration of matrix rows and columns #2177
5. Thanks!

Is it ok for you to just handle the first three checkboxes in this PR, and address the other checkboxes in separate PR's?

No problem!

Hm, the @babel/transform-runtime configuration gave issues, see #2169. I've reverted this for now, but we should find a solution in order to be able to use iterators.

I don't have many experiences with Babel (and those that I do have are mostly negative 😁️), however if there's something specific I can help with, let me know.

[josdejong]

Thanks for the updates, and for starting separate topics for the separate ideas you have.

I don't have many experiences with Babel (and those that I do have are mostly negative 😁️)

😂 yeah same here... I wish it would "just work"

I was thinking about matrixFromRows and matrixFromColums: this should ideally play nice with the methods matrix.rows() and matrix.columns() that you're proposing in #2177, since it's is sort of the opposite.

Do you have any real world examples of where you need to pull a matrix apart in rows/columns, and afterwards glue it together again? I want to be careful to implement features that have a clear real-world use, and make sure new features really align with real-world usage. In this case, we could go for the features matrixFromRows, matrixFromColums, .rows(), .columns(). But I can also imagine that it is just "fine" to use plain old for loops to iterate over a matrix, and to put rows/columns together again using concat.

[cshaa]

Do you have any real world examples of where you need to pull a matrix apart in rows/columns, and afterwards glue it together again?

Basis transformations are the first thing that comes to my mind – if you work in a basis uᵢ, therefore [u₁]ᵤ = (1,0,...), [u₂]ᵤ = (0,1,...), ..., and want to change to a basis vᵢ, the transformation matrix for vectors is made of columns [vᵢ]ᵤ. Conversely, if you have a transformation matrix and don't know the basis vectors, you can get them as the columns of the matrix.

There are several other use cases, like extracting eigenvectors from the matrix that eigs returns. Or, if you're interested in the image of a matrix, it is spanned by its columns, so there are potentially things to do with that.

If you had three vectors (column matrices) v1, v2 and v3 and wanted to make a transformation matrix like this with the old API, it would be quite annoying – you'd have to convert them to flat arrays, stack those into one two-dimensional array, make a matrix out of it and finally transpose the matrix:

const v1, v2, v3
let M = transpose(matrix([ flatten(v1.toArray()), flatten(v2.toArray()), flatten(v3.toArray()) ]))

... or something like that. The code is needlessly complicated, although the actual task is almost trivial. And it's definitely not self-documenting, I'd have a hard time understanding what it does even in my own codebase. Compare that to:

let M = matrixFromColumns(v1, v2, v3)

I couldn't think of an example where one would want to take the columns, do something with them and then recombine them into a matrix again – maybe if you have a transformation matrix and want to change the basis you're transforming into. However, as I pointed out, there are legit use cases for doing those things separately – either taking columns of a matrix, or constructing a matrix from columns.

So far, the examples have been about columns. I think matrixFromColumns and columnsOf are more important than their row counterparts, both because columns are used more frequently (matrices from rows correspond to transformations of covector basis, covectors aren't used as frequently as vectors) and because working with rows is much easier even without a dedicated API. However, only providing these methods for columns would imo feel arbitrary API-wise.

[josdejong]

Thanks, you convinced me 😄 . I agree that having the tooling to pull matrices apart and put them together in various ways are basic needs which can make life easy. I think if we provide these methods for columns, we should indeed also offer them for for rows else the API is incomplete (even if the need may be less).

Thinking about my earlier comment:

I was thinking about matrixFromRows and matrixFromColums: this should ideally play nice with the methods matrix.rows() and matrix.columns() that you're proposing in #2177, since it's is sort of the opposite.

If we implement matrix.rows() and matrix.columns() in #2177 as iterators, I think there is no need to directly have matrixFromRows and matrixFromColums support that too: I think you can just use the spread operator there, or convert the iterator into an Array yourself one way or another.

So I think this PR is ready to be merged functionally wise, only we still have to fix the Babel issues 😕 .

[cshaa]

If we implement matrix.rows() and matrix.columns() in #2177 as iterators, I think there is no need to directly have matrixFromRows and matrixFromColums support that too: I think you can just use the spread operator there, or convert the iterator into an Array yourself one way or another.

Having rows and columns return an iterable and then using the spread operator in matrixFromRows and -Columns sounds good to me 👍️

So I think this PR is ready to be merged functionally wise, only we still have to fix the Babel issues.

I tried adding @babel/runtime. It seems to have worked. But it means one runtime dependency more, I'm curious how this will impact bundle size. I guess we'll see with the next version: https://bundlephobia.com/result?p=mathjs

[josdejong]

I tried adding @babel/runtime. It seems to have worked. But it means one runtime dependency more, I'm curious how this will impact bundle size. I guess we'll see with the next version: https://bundlephobia.com/result?p=mathjs

We should not need @babel/runtime as a dependency, should only be needed at build time. I think we may need to have different babel configs for either esm/cjs output, and for bundle output. It's on my list to try figure this out but haven't had time so far.

[cshaa]

We should not need @babel/runtime as a dependency, should only be needed at build time.

I'm not sure I understand... We already have runtime dependencies, why should @babel/runtime be treated differently?

Initially, I was also skeptical about having a Babel as a dependency after what happened with left-pad, but @babel/runtime only has one dependency (regenerator-runtime), which itself has no dependencies. And both of these packages are critical for a lot of software, and both are owned by the co-creator of Babel – so, it's not overly dependent on the NPM ecosystem.

Therefore I don't think there's anything wrong with having Babel Runtime as a dependency, apart from the obvious fact that it's one dependency more than we had before.

EDIT: Those dependencies are “runtime dependencies” in the sense that when you compile your project for the browser, they will be bundled together with it. It's not like you have to do anything else than include lib/browser/math.js when you want to use the library.

[josdejong]

I'm not sure I understand... We already have runtime dependencies, why should @babel/runtime be treated differently?

You may be right, it may be not a problem at all.

To me @babel/runtime is a bit similar to ES5/ES6 polyfills for say Promise, which where needed for older browsers like IE. In our case, we want to use generators now, which are supported by all JavaScript engines except IE11. Converting the use of the native builtin generator with @babel/runtime feels odd. I found this explanation really clear btw: https://stackoverflow.com/a/37871233/1262753

I guess adding @babel/runtime as a dependency (not devDependency) is best for now. As soon as we don't have to support engines that lack support for generators we can configure Babel/Webpack to leave generator code as is and remove this dependency again.

Ok then will merge your PR now 👍

[josdejong]

@m93a when testing I noticed one more thing that we may want to adjust: function matrixFromFunction currently always returns a Matrix. I think it makes sense to let it return an Array when the input size is an Array, that would be consistent with all other functions in mathjs. What do you think?

[cshaa]

Aaah, I hate “array in—array out, Matrix in—Matrix out” 😂️ But yes, you're probably right, it would be more consistent with the rest of the library if the return type were array for size: number[] and Matrix for size: Matrix.

[josdejong]

👍 lets keep it consistent for now then.

We can open a separate topic to discuss the “array in -> array out, Matrix in -> Matrix out”, I would like to better understand what you dislike about it, and if you have any alternative ideas in this regard.

[josdejong]

Published in v9.4.0, thanks again!