josdejong
Change function kron to output a 1d vector when input is 1d
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: https://github.com/josdejong/mathjs/issues/230#issuecomment-591319887
Note this only applies when both inputs are 1d; when either input is 2d, the current behavior appears to be correct
@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?
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.
@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])?
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!
@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?
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
@gwhitney I've raised PR #3133, please let me know if I need to add something or it can be approved.