Change function kron to output a 1d vector when input is 1d #3133
Change function kron to output a 1d vector when input is 1d #3133Kiki-1234-web wants to merge 1 commit intojosdejong:developfrom
Conversation
|
Oh, sorry, I misunderstood the discussion in #1753. It should be the case that |
|
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. |
|
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. |
josdejong
left a comment
There was a problem hiding this comment.
@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?
|
|
||
| it('should calculate product for empty 2D Arrays', function () { | ||
| assert.deepStrictEqual(math.kron([[]], [[]]), [[]]) | ||
| assert.deepStrictEqual(math.kron([[]], [[]]), []) |
There was a problem hiding this comment.
Since the inputs are 2d arrays, the output should stay a 2d array [[]] like it was before.
| assert.deepStrictEqual(math.kron([1, 1], [[1, 0], [0, 1]]), [[1, 0, 1, 0], [0, 1, 0, 1]]) | ||
| assert.deepStrictEqual(math.kron([[1, 0], [0, 1]], [1, 1]), [[1, 1, 0, 0], [0, 0, 1, 1]]) | ||
| assert.deepStrictEqual(math.kron([1, 2, 6, 8], [12, 1, 2, 3]), [[12, 1, 2, 3, 24, 2, 4, 6, 72, 6, 12, 18, 96, 8, 16, 24]]) | ||
| assert.deepStrictEqual(math.kron([1, 2, 6, 8], [12, 1, 2, 3]), [12, 1, 2, 3, 24, 2, 4, 6, 72, 6, 12, 18, 96, 8, 16, 24]) |
There was a problem hiding this comment.
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?
|
@Kiki-1234-web can you have a look at the feedback? |
|
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:
***@***.***>
|
|
Thanks! |
|
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 ? |
|
Yes let's close this PR. We can always pick it up again but there is simply is no action on it right now. |
When both inputs are 1D vectors, kron function return 1D vector as an output.