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

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Kiki-1234-web
Copy link

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

@gwhitney
Copy link
Collaborator

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
Copy link
Author

Kiki-1234-web commented Jan 23, 2024

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
Copy link
Collaborator

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.

Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

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

@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?

@@ -29,13 +29,12 @@ describe('kron', function () {
})

it('should calculate product for empty 2D Arrays', function () {
assert.deepStrictEqual(math.kron([[]], [[]]), [[]])
assert.deepStrictEqual(math.kron([[]], [[]]), [])
Copy link
Owner

Choose a reason for hiding this comment

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

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

it('should calculate product for 1D Arrays', function () {
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])
Copy link
Owner

Choose a reason for hiding this comment

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

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
Copy link
Owner

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

@Kiki-1234-web
Copy link
Author

Kiki-1234-web commented Feb 14, 2024 via email

@josdejong
Copy link
Owner

Thanks!

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.

3 participants