Fix to Issue #2637, and also added support for product of Matrix and Vector
In essence, the Vector Vector Kronecker product was written as an array comprehension, but array comprehensions return 2D arrays, so I added an explicit vectorization after the list comprehension and reordered the comprehension indices to get the proper result.
Tests were added covering all these cases.
Previous pull request #2643 had bug in the implementation of Matrix/Vector Kronecker product, which should have been caught by the tests added if I had run them properly.
Fixed Kronecker product of column vectors
Array comprehension returns a 2D array, so I added an explicit
vectorization after the list comprehension and reordered the
comprehension indices to get the proper result.
Multiple fixes to kron
Fixed issue 2637, and added support of Kronecker product between
Vectors and Matrices.
Added tests for Kronecker product
Added simple tests for Kronecker product involving Matrices,
Vectors and scalars.
Fixed bug in my implementation of Matrix/Vector Kronecker product
You can push additional commits onto a branch without opening a new pull request, by the way.
I figured that out after I submitted this request. Still learning the ropes ...
No problem! I'll leave this for someone more familiar with the linalg code to review/merge: @andreasnoackjensen @ViralBShah @dmbates off the top of my head.
Thank you for pointing to the missing/wrong methods. However, the new versions will create many array allocations. I haven't benchmarked but I think the original completely devectorized matrix-matrix version is about the fastest way to do the Kronecker. A solution to the vector versions could be something like
I don't have a good intuition about which way the performance will go with the version I have here, but I will run some benchmarks to double check.
You don't want to do res = hcat(res, ...) since that will do O(n^2) copying.
res = hcat(res, ...)
Simplified Vector/Vector kron and reverted Matrix/Matrix
Simplified Vector/Vector `kron` following
@andreasnoackjensen's suggestion, and reverted the changes to
Matrix/Matrix `kron` due efficiency of implementation, as pointed
out by @JeffBezanson and @andreasnoackjensen.
Added support for Matrix/Vector & Vector/Matrix kron
Both falling back on the Matrix/Matrix case already implemented
I reverted the changes to the previous version of Matrix/Matrix kron, and used the simpler approach to the Vector/Vector, Vector/Matrix, Matrix/Vector cases as suggested by @andreasnoackjensen
@ViralBShah I think this one is ready for merge.