Broadcasting functions by dvd101x · Pull Request #3624 · josdejong/mathjs

dvd101x · 2025-12-26T16:06:58Z

Hi this is according to discussion #3516

It's missing some tests and type checking.

…perations

gwhitney · 2025-12-27T22:50:08Z

OK, I will convert to draft and you can mark it as ready for review when you feel you've supplied all the missing bits.

…rays

dvd101x · 2025-12-29T04:48:53Z

Hi, this is ready for review.

gwhitney

I can't comment on the utility/advisability of exposing these functions in the top-level user interface of mathjs,but I am assuming that's something you've already worked out with Jos. So I am explicitly not attempting to judge whether these functions should be added. Presuming they should be, here's my review of the PR to add them.

src/expression/embeddedDocs/function/matrix/broadcastMatrices.js

gwhitney · 2026-01-03T07:36:34Z

src/expression/embeddedDocs/function/matrix/broadcastSizes.js

+  syntax: [
+    'broadcastSizes(sizeA, sizeB)'
+  ],
+  description: 'Broadcast the sizes of matrices to a compatible size',


Similarly, I am concerned about at least the grammatical correctness here. It's not that any sizes are being broadcast here, per se, is it, but rather that the size resulting from a broadcast is being computed, right? So shouldn't the description be something more like "Compute the size that would result from broadcasting a list of matrices of the given sizes, if possible"? (Again, this function can throw an error if the sizes are incompatible, correct?)

This also observation also, for me, calls into question the name of the function. Would broadcastSize or sizeOfBroadcast be more descriptive, again since no sizes are being broadcast, per se?

The terminology used by numpy is

numpy.broadcast_shapes(*args)
Broadcast the input shapes into a single shape.

I think I understand where are you coming from, because the original sizes are kept intact. But maybe it's an implicit definition, because when one adds numbers, nothing happens to the numbers, we could say is to compute the result from adding numbers.

Yes this function will throw an error for incompatible sizes.

No, I meant that it's the matrices that are broadcast, not their sizes. This function does not actually do any broadcasting. It just computes a size, and so it should be named accordingly, I think. Your thoughts?

Any further thoughts here? I am recommending:

rename the function to either sizeOfBroadcast or broadcastSize, and

describe it as Compute the size of a matrix that would result from broadcasting matrices of the given sizes, if they are compatible

because, as a matter of the plain meanings of the words, a matrix/array can be broadcast, but a size cannot be broadcast.

Sorry, even though I had a response planned from the first comment I was debating myself on how to present the response.

Broadcasting, when applied to a matrix's shape, does not imply treating the shape itself as a data structure to be broadcasted. Much like the phrase 'running water' describes flow rather than the physical stride of a 'running person,' broadcasting a shape is a functional convention.

I don't know if the convention all comes from numpy, but it's a common trend stdlib: broadcast-shapes

I think the name of the function is ok as is, but it could be described as you suggest to avoid a circular definition.

Sure, we should just strive for both the clearest name of the function and the clearest description of what it does. If you think that "broadcastSizes(s, t, u)" is clearer than "broadcastSize(s, t, u)", I am not going to quibble. But I do prefer a more explicit description, thanks.

gwhitney · 2026-01-03T07:41:29Z

src/expression/embeddedDocs/function/matrix/broadcastTo.js

@@ -0,0 +1,15 @@
+export const broadcastToDocs = {
+  name: 'broadcastTo',


I worry about the name of this function. It seems to me that since sizes look like matrices, visually, broadcastTo([3], [2, 2]) could look like it is supposed to broadcast the first matrix to be compatible with the second, i.e. produce [3,3] rather than [3, 3; 3, 3]. I would strongly recommend considering renaming the function to broadcastToSize([3], [2, 2]) to avoid this ambiguity.

I understand. Many of these are taken from numpy and have counterparts in jax / mlx / pytorch and maybe others.

numpy.broadcast_to(array, shape, subok=False)
Broadcast an array to a new shape.

I don't have a strong opinion on this, just please review if it makes sense to follow that convention.

I am the one less familiar with the territory here. That's why this was couched as a suggestion. Please select the name you think is best, including leaving it be, unless @josdejong weighs in otherwise. Please just post your final decision here.

Again, to land this PR we need a decision on the final name here. If you are on the fence, I recommend switching to broadcastToSize() in an effort to steer away from the ambiguity I raised. It seems to me a more fully specified name can't be harmful here. (And I don't think we need to worry too much about fidelity to numpy names, since after all to begin with what they call a "shape" we call a "size" so we're not adopting numpy terminology right from the start.)

I agree about the differences between shape and size. I'm considering broadcastToSize(), will review and resolve this comment.

gwhitney · 2026-01-03T07:48:19Z

src/function/matrix/broadcastMatrices.js

+export const createBroadcastMatrices = /* #__PURE__ */ factory(name, dependencies, ({ typed }) => {
+  /**
+   * Broadcast multiple matrices together.
+   * Return and array of matrices with the broadcasted sizes.


Typo: "and" -> "an"

This documentation is way too terse for someone who's not already familiar with the operation of broadcasting matrices (which is not necessarily all that common or standard) to understand what is going on. Somewhere in the documentation needs to be a careful documentation from the ground up with examples what it means to broadcast two or more matrices. That could be here, or it could be elsewhere (like in the general matrix documentation page) and then be linked to here. Such documentation might already exist, and then all you need is a link.

This documentation should say what happens with incompatible sizes.

Finally, you have "sizes" plural. But isn't it the case that there is only one common size produced by broadcasting a list of matrices?

OK, the typo/grammar issues are gone, but so is any explanation, so far as I can see. Either the broadcasting operation needs to be described in detail here, or there needs to be a link to somewhere that it is described. The documentation cannot simply assume that the reader knows what it means to "broadcast matrices against each other." That's not a standard, well-known operation. Also, there needs to be a description of the compatibility requirements on the matrices and what happens if those requirements don't hold.

gwhitney · 2026-01-03T07:50:50Z

src/function/matrix/broadcastSizes.js

+
+export const createBroadcastSizes = /* #__PURE__ */ factory(name, dependencies, ({ typed }) => {
+  /**
+   * Calculate the broadcasted size of one or more matrices or arrays.


As per my comments on the internal docs, shouldn't this be something more like "Calculate the size that would result from broadcasting one or more matrices or arrays, given the sizes of the input collections."?

The same comments about having documentation on the operation of broadcasting either here or linked here apply to this function as well. Also mention of what happens with incompatible sizes.

Still needs further editing/documentation.

gwhitney · 2026-01-03T07:55:07Z

test/unit-tests/utils/array.test.js

@@ -702,9 +702,9 @@ describe('util.array', function () {
    })

    it('should broadcast leave arrays as such when only one is supplied', function () {


I know you didn't create these problems, but there are typos/ungrammaticality in the labels of both this test and the following one. Please fix.

gwhitney · 2026-01-03T07:56:16Z

types/index.d.ts

+  /**
+   * Broadcast a matrix or array to a specified size.
+   *
+   * The input collection is conceptually expanded to match the given dimensions,


Maybe instead "entries of the input collection are duplicated to match the given size," ?

gwhitney · 2026-01-03T07:56:44Z

types/index.d.ts

+   *
+   * The input collection is conceptually expanded to match the given dimensions,
+   * following broadcasting rules. The returned object is a new matrix or array
+   * with the requested size; the original input is not modified.


Where do I find these "broadcasting rules"?

Good question, I don't think broadcasting is described with specific rules, the chapter can be found at broadcasting.

The best source I've found is from numpy.
https://numpy.org/doc/stable/user/basics.broadcasting.html
there is one from octave
https://docs.octave.org/latest/Broadcasting.html#Broadcasting-1

I personally didn't know about this topic until a few years ago even after using Matlab/Octave extensively. The links I'm sharing is not an assumption of anyone's knowledge, just sharing them to try to answer the question.

I don't know what would be best, extend the chapter, have a better phrasing of "broadcasting rules" or something else.

Good question, I don't think broadcasting is described with specific rules, the chapter can be found at broadcasting.

All good. Just make that phrase "broadcasting rules" a link to that spot in the on-line docs, and make any fixes/additions you deem valuable to that section on broadcasting (for example, at least the first example needs to be corrected, as [1,2] + 3 = [4,5], not [3,4] as shown). Then all will be well. Similaly, the doc sections in the broadcast functions themselves should link to that broadcasting link. Thanks!

src/function/matrix/broadcastSizes.js

src/function/matrix/broadcastTo.js

dvd101x · 2026-01-04T03:35:42Z

... but I am assuming that's something you've already worked out with Jos.

Yes. I took this comment as an OK.

#3516 (reply in thread)

Part of the argument is that these are exposed by numpy even if broadcasting is deeply integrated. Also during the implementation of broadcasting there were some discussions about the specific functions.

#2753 (comment)

#2895

I think this means it's ok, but if not please let me know.

gwhitney · 2026-01-04T20:35:16Z

I took this comment as an OK.

Yes, you convinced (an initially skeptical) Jos so all OK :)

josdejong · 2026-01-07T11:52:35Z

Glen, thanks for reviewing the work of David.

I indeed think it's a good idea to add these functions.

src/expression/embeddedDocs/embeddedDocs.js

gwhitney · 2026-03-01T07:40:11Z

src/expression/embeddedDocs/function/matrix/broadcastMatrices.js

+  category: 'Matrix',
+  syntax: [
+    'broadcastMatrices(A, B)'
+  ],


Should the syntax read 'broadcastMatrices(A, B, ...)' since any number of arguments are allowed?

gwhitney · 2026-03-01T08:12:22Z

src/function/matrix/broadcastTo.js

+      const result = M.create()
+      result._size = size.valueOf()
+      result._data = broadcastTo(M.valueOf(), size.valueOf())
+      result._datatype = M.datatype()


We don't want to be breaking encapsulation of the internal format of matrices here. In particular, this implementation is supposed to work with an arbitrary Matrix implementation, based on its typed-function signature. Hence it can't delve into the internal fields, as they are assuming M is a DenseMatrix, which it might not be.

Therefore, this should be M.create(broadcastTo(M.valueOf(), size.valueOf()), M.datatype()). I understand your concern about unnecessarily recomputing the size. If you really want to get around that, there are some options:

Decide that broadcasting always returns a DenseMatrix, and use DenseMatrix creation methods that take your word for the size, if there are any such methods. This plan might not be wise at a time when you/we are contemplating adding other general-purpose matrix implementations besides DenseMatrix, creating a world in which we would not want operations to capriciously convert back into DenseMatrix.

Extend the interface of M.create() to optionally take a guaranteed size and/or other validation-short-circuiting options. That might require a number of coordinated changes in the Matrix classes.

Of course I am open to other ideas. But we don't want DenseMatrix-internals-specific code here. This observation also suggests there should be unit tests for the broadcasting functions on SparseMatrix arguments. Please add some if they aren't there.

For this case I think it can be left as you mention M.create(broadcastTo(M.valueOf(), size.valueOf()), M.datatype()). Maybe for the future some options could be added to the create matrix to skipCloning, skipValidation, skipPreProcess, etc.

dvd101x added 5 commits December 9, 2025 22:38

feat: implement broadcastSizes and broadcastTo functions for matrix o…

6b0ea89

…perations

Included new functions in factories

cd83def

Fixed typed issues

f2a2943

format

756aa83

Added embedded docs

a804f9d

gwhitney marked this pull request as draft December 27, 2025 22:50

dvd101x added 6 commits December 27, 2025 22:39

Added tests for broadcastSizes

cfe6490

Added tests for broadcastMatrices and fixed an issue with broadcastAr…

5224a45

…rays

Merge branch 'develop' into broadcasting-functions

b212e6f

Added test for broadcastTo

4315651

Added more tests to broadcastTo

c99c48a

Add types

d749405

dvd101x marked this pull request as ready for review December 28, 2025 20:28

dvd101x added 3 commits December 28, 2025 15:23

Fixed wrong example in jsdocs

c267a8c

Added hisotry

23bc583

Format

26367b5

gwhitney requested changes Jan 3, 2026

View reviewed changes

josdejong reviewed Jan 7, 2026

View reviewed changes

src/expression/embeddedDocs/embeddedDocs.js Outdated Show resolved Hide resolved

dvd101x added 4 commits January 8, 2026 21:28

Fix typos and grammar errors

54338f5

Merge branch 'develop' into broadcasting-functions

5e13b30

Merge branch 'develop' into broadcasting-functions

d593869

Merge branch 'develop' into broadcasting-functions

41d2db1

gwhitney reviewed Mar 1, 2026

View reviewed changes

		@@ -0,0 +1,15 @@
		export const broadcastToDocs = {
		name: 'broadcastTo',

		@@ -702,9 +702,9 @@ describe('util.array', function () {
		})

		it('should broadcast leave arrays as such when only one is supplied', function () {

Uh oh!

Conversation

dvd101x commented Dec 26, 2025

Uh oh!

gwhitney commented Dec 27, 2025

Uh oh!

dvd101x commented Dec 29, 2025

Uh oh!

gwhitney left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwhitney Jan 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dvd101x commented Jan 4, 2026

Uh oh!

gwhitney commented Jan 4, 2026

Uh oh!

josdejong commented Jan 7, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gwhitney Jan 3, 2026 •

edited

Loading