Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add kron(): kronecker product for 2D matrices #1039

Closed
wants to merge 1 commit into from

Conversation

notmgsk
Copy link

@notmgsk notmgsk commented Jun 20, 2021

Closes #652

H/T @ethanhs

Copy link
Contributor

@emmatyping emmatyping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@bluss bluss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks straightforward

///
/// The kronecker product of a LxN matrix A and a MxR matrix B is a (L*M)x(N*R)
/// matrix K formed by the block multiplication A_ij * B.
pub fn kron<T>(a: &Array2<T>, b: &Array2<T>) -> Array2<T>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To include this we'd need to use the same kind of argument type we usually use for reading an array - an &ArrayBase like other places that take any kind of array.

Minor style issues - I'd suggest it can have the slightly longer name kronecker. Also note that ndarray uses the element type parameter A by convention, so we need to keep that to keep it consistent. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think kron would align with numpy, as a datapoint, just wanted to mention. It also is an operation usually done on 2 dimensional matrices, though I believe numpy supports higher dimensional kronecker products.

.into_iter()
.zip(a.iter())
{
let v: Array2<T> = Array2::from_elem((dimbr, dimbc), *(elem)) * b;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we'd like to avoid creating an intermediate array. We'd just use Zip instead of from_elem and assign. Other improvements are less important (alloc uninit array instead of zeroed, other ways to loop?). We would however like to replace the other zip with Zip because the iteration order of exact_chunks_mut is not well specified.

Copy link
Member

@bluss bluss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, request changes would be the rather better summary. 🙂

@emmatyping
Copy link
Contributor

@notmgsk Are you interested in continuing to work on this? If not I will probably build off of this PR and respond to the review.

@emmatyping
Copy link
Contributor

I went ahead and made #1105, which I think responds to the feedback here.

@bluss
Copy link
Member

bluss commented Nov 11, 2021

Superseded by #1105

@bluss bluss closed this Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kronecker product
3 participants