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

[josdejong]

Currently the function kron outputs a 2d matrix for both 1d and 2d input. It would be more consistent when the output is 1d for 1d input. This will be a breaking change though.

For reference: #230 (comment)

[gwhitney]

Note this only applies when both inputs are 1d; when either input is 2d, the current behavior appears to be correct

[Kiki-1234-web]

@josdejong @gwhitney Regarding the issue, I have made changes, if two vectors are 1D then they are returning 1D vector only, although I have two queries in the unit test.
In case of two empty arrays, right now it’s returning two empty arrays one inside another. However, after fixing the issue it must return one empty array.  
Another unit test it’s failing expected output:
[ [12, 1, 2, 3],
[24, 2, 4, 6],
[72, 6, 12, 18],
[96, 8, 16, 24] ]
actual output :
[ [ 12, 1, 2, 3 ],
[ 24, 2, 4, 6, 72, 6, 12, 18 ],
[ 96, 8, 16, 24 ]
]
I would like to raise the PR after making changes in unit tests, can I do that?

[gwhitney]

Kronecker product of two empty arrays being an empty array seems right.
For the other, can you point us to the exact location of the unit test in question? I glanced at kron.test.js and didn't see it, but maybe I just missed it. Thanks.

[Kiki-1234-web]

@gwhitney
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]])
right now the above unit test is returning 2-D matrix.
According to the change in code and requirement, am I supposed to return 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])?

[gwhitney]

Ah, now I see it; sorry it wasn't clear before. Yes, the agreed-upon behavior change is to remove one level of brackets in the expected output of this case. So you can go ahead and make that change to the unit test as well. If you feel there are any other tests that would exercise cases of interest in the new code, feel free to add them as well. Thanks!

[Kiki-1234-web]

@gwhitney do I need some sort of access to push the changes in my local branch taken out from develop branch and raise a PR?
git push
remote: Repository not found.
fatal: repository 'https://github.com/josdejong/mathjs.git/' not found
I've tried all possible solutions and last one is to confirm if I need access from to push?

[gwhitney]

No, you fork the repository, make a branch in your fork, and raise the PR from your fork. Guide at https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork

[Kiki-1234-web]

@gwhitney I've raised PR #3133, please let me know if I need to add something or it can be approved.

[josdejong]

Thanks @Kiki-1234-web, I'll review the PR later this week.

[Rachit2912]

is this issue still open ?

[gwhitney]

There was a PR #3133 that purported to resolve this issue; on review there was feedback to the submitter requesting changes to the PR; those changes have not to date been made; hence, yes, this issue remains open at this point.