Broadcasting #2895
Broadcasting #2895
Conversation
missing dot in `A._size`
|
Thanks David, this looks good at first sight! I'll review your PR thoroughly within a few days. Some first feedback: I see the tests only cover 54% of the code of broadcast.js right now (see codecov report: https://app.codecov.io/gh/josdejong/mathjs/pull/2895). Do you plan on adding more tests? |
|
Hi Jos, thanks for the review. I have one question, if I include tests for the function add, subtract, etc. that uses broadcast, would it increase the broadcast test cover percentage? At this moment I don't know if the broadcasting tests can be done on the functions that uses broadcast or if there it's needed to make a new test. |
|
I just found an issue on an edge case, I'm doing another update that will be ready today. |
There was a problem hiding this comment.
This works like a charm David, and the code is very neat and readable, nice job 😎!
I made a couple of small inline comments, can you reply to those? And two more feedbacks:
- Can you add a section in the documentation about matrices explaining broadcasting, I think
docs/datatypes/matrices.mdwould be the most logic place? - Looking at the code I do not thing the broadcasting will have a negative impact on the existing operations, it adds a check on the matrix size but that's it. Do you have the same idea?
- There is a small merge conflict now due to the change in AUTHORS, can you rerun
npm run update-authorsand fix that?
|
For reference: this PR addresses #2753 |
|
Hi Jos, I'm very glad it's working well. I did respond to the inline comments and worked on them.
I had some questions initially but I think they got resolved by working on them, thus I resolved the conversations except for the one about mutation and cloning.
I really like the shape it's taking, thanks for the through and clear review! |
|
Thanks for the updates.
Yeah me too! Is this a word joke? Broadcast is all about shape 😉 |
|
Hahaha it wasn't a joke but it should have been. From my side now it's clear that a clone was necessary to avoid mutation as you mentioned, the code currently reflects that and I think if no new topic arises the code might be ready. |
|
Thanks for writing the docs and resolving all further conversations! Can you resolve the conflict with the AUTHORS file? I think besides that the PR is ready to merge :) |
|
That's great news. I did run |
|
🤔 did you update your branch? There is a conflict with the AUTHORS file currently in |
|
I haven't updated my branch, maybe that's why There seems to be an issue that I don't know how to resolve.
Right now I think I should discard everything, edit and push again, but I don't really know if that's correct. |
|
I think the easiest is to just undo the changes in |
|
I've fixed the commit now in-browser, will merge the PR after the CI is done |
|
Published now in |
* broadcasting * Simplified broadcasting * Updated for broadcasting * Changed to camel case * Camel case and auto formating * Added comments * Skip if matrices have the same size * Fixed issue with undefined variable missing dot in `A._size` * Implemented broadcasting in all functions * Added helper functions * Added function to check for broadcasting rules * Tests for broadcasted arithmetic * Fixed issue with matrix the size of a vector * Documented and updated broadcasting * Included broadcast.test --------- Co-authored-by: David Contreras <david.contreras@guentner.com> Co-authored-by: Jos de Jong <wjosdejong@gmail.com>
* #2567: accept array as parameter for gcd() * #2567: accept 1d matrix as gcd() argument * #2567: support nested 1d array in gcd * #2567: simplify matrix signature * [fix] intersect method parameter type (#2897) * Update history and authors (see #2897) * feat: added chirp-z transform to calculate non-power-of-2 fft (#2900) * added chirp-z transform to calculate non-power-of-2 fft * simplify/remove _ifft function inside _czt function * chore: remove an unused dependency from `simplifyConstant` * fix: quantileSeq not accepting a matrix as second argument `prob` (see #2902) * fix a broken example of function `to` * fix a typo in the examples functions `distance`, `getMatrixDataType`, `subset`, and `max` (see #2902) * fix linting issue * Broadcasting (#2895) * broadcasting * Simplified broadcasting * Updated for broadcasting * Changed to camel case * Camel case and auto formating * Added comments * Skip if matrices have the same size * Fixed issue with undefined variable missing dot in `A._size` * Implemented broadcasting in all functions * Added helper functions * Added function to check for broadcasting rules * Tests for broadcasted arithmetic * Fixed issue with matrix the size of a vector * Documented and updated broadcasting * Included broadcast.test --------- Co-authored-by: David Contreras <david.contreras@guentner.com> Co-authored-by: Jos de Jong <wjosdejong@gmail.com> * Update history and authors * Update devDependencies * publish v11.6.0 * fix #2906: improve description of the behavior of `subset` for scalar values in the docs * fix #2907: determinant of empty matrix should be 1 * chore: add a few more unit tests to `det` --------- Co-authored-by: Jos de Jong <wjosdejong@gmail.com> Co-authored-by: Jaeu Jeong <wodndb@gmail.com> Co-authored-by: cyavictor88 <100557319+cyavictor88@users.noreply.github.com> Co-authored-by: David Contreras <dvd.cnt@gmail.com> Co-authored-by: David Contreras <david.contreras@guentner.com>
This is an implementation of broadcasting. It stretches matrices to make them the same size if possible according to the rules of numpy and Matlab.
A future development would be to modify indexing in element wise matrix operations of two or more matrices to make it unnecessary to stretch the matrices as it takes unnecessary memory.