Change function kron to output a 1d vector when input is 1d

[Kiki-1234-web]

When both inputs are 1D vectors, kron function return 1D vector as an output.

[gwhitney]

Oh, sorry, I misunderstood the discussion in #1753. It should be the case that kron([],[]) equal [] (you may want to add that as a test). But even with the intended behavior change, the kronecker product of two 2-d arrays with no entries kron([[]],[[]]) should be a 2-d array with no entries [[]]. The implementation of #1753 should only change the behavior of the kronecker product when both arguments are 1-d arrays. So I don't think the PR is quite ready to merge as it stands. Also, once it is ready, it should be marked as resolving #1753. Thanks!

[Kiki-1234-web]

Since we don't deal with matrices above size 2 here, I believe edge case handling, especially for 2D-empty matrices, would be appropriate. Though it may sound naive, suggestions are welcomed.

[gwhitney]

I will be frank, the fact that your current change is failing this empty 2D matrix test suggests to me that you're checking for 1D inputs in the wrong place (but I am not sure of that, I haven't tried to debug the code/find the right place). It also suggests to me that there need to be additional tests. For example, the kronecker product of two 1-by-1 matrices should be a 1-by-1 matrix, e.g. kron([[2]],[[3]]) should be [[6]] -- you should check that you are getting that right. Also a 1-by-2 and a 1-by-3 should give you a 1-by-6, e.g. kron([[1,2]],[[1,2,3]]) should be [[1,2,3,2,4,6]] -- that should likely get added to the tests. For symmetry, we might want to check that kron([[1],[2]],[[1],[2],[3]]) comes out to [[1],[2],[3],[2],[4],6]] and do ones that go opposite directions as well: kron([1,2], [[1],[2],[3]]) should be [[1,2],[2,4],[3,6]] and kron([[1],[2]], [[1,2,3]]) should be kron([[1,2,3], [2,4,6]]). Once you have all of those working and the empty case as well, then much more likely the PR is ready to merge.

[josdejong]

@Kiki-1234-web . Thanks for your PR. I had a look, and indeed like Glen explained, the new behavior doesn't yet match what we're looking for. In brief:

• 1d inputs -> 1d output
• 2d inputs -> 2d output
• a mix of 1d and 2d inputs -> 2d output

Does that make sense?

[josdejong]

Since the inputs are 2d arrays, the output should stay a 2d array [[]] like it was before.

[josdejong]

This is indeed the change we want 👌. Now, the name of the test is a bit misleading. Can you split the tests? One testing 2d inputs, one testing a mix of 1d and 2d input, and one testing 1d inputs?

[josdejong]

@Kiki-1234-web can you have a look at the feedback?

[Kiki-1234-web]

Sure, I looked at it although got little busy with personal commitments. Will get back to it in a few days.

…

On Wed, 14 Feb 2024 at 1:30 PM, Jos de Jong ***@***.***> wrote: @Kiki-1234-web <https://github.com/Kiki-1234-web> can you have a look at the feedback? — Reply to this email directly, view it on GitHub <#3133 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVMIDYJUP53IAMJFBDJ32R3YTRVINAVCNFSM6AAAAABCEEEDUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBTGI2DONZSG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[josdejong]

Thanks!

[gwhitney]

With no activity for a year and a lack of evidence that this PR has the correct approach for the relevant issue, I would vote to simply close this PR. @josdejong ?

[josdejong]

Yes let's close this PR. We can always pick it up again but there is simply is no action on it right now.