Skip to content
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/expression/embeddedDocs/embeddedDocs.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,9 @@ import { partitionSelectDocs } from './function/matrix/partitionSelect.js'
import { rangeDocs } from './function/matrix/range.js'
import { reshapeDocs } from './function/matrix/reshape.js'
import { resizeDocs } from './function/matrix/resize.js'
import { broadcastMatricesDocs } from './function/matrix/broadcastMatrices.js'
import { broadcastToDocs } from './function/matrix/bradcastTo.js'
Copy link
Owner

Choose a reason for hiding this comment

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

I think this filename bradcastTo has a typo and should be broadcastTo

import { broadcastSizesDocs } from './function/matrix/broadcastSizes.js'
import { rotateDocs } from './function/matrix/rotate.js'
import { rotationMatrixDocs } from './function/matrix/rotationMatrix.js'
import { rowDocs } from './function/matrix/row.js'
Expand Down Expand Up @@ -481,6 +484,9 @@ export const embeddedDocs = {
partitionSelect: partitionSelectDocs,
range: rangeDocs,
resize: resizeDocs,
broadcastMatrices: broadcastMatricesDocs,
broadcastTo: broadcastToDocs,
broadcastSizes: broadcastSizesDocs,
reshape: reshapeDocs,
rotate: rotateDocs,
rotationMatrix: rotationMatrixDocs,
Expand Down
15 changes: 15 additions & 0 deletions src/expression/embeddedDocs/function/matrix/bradcastTo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export const broadcastToDocs = {
name: 'broadcastTo',
category: 'Matrix',
syntax: [
'broadcastTo(A, size)'
],
description: 'Broadcast a matrix to a compatible size',
examples: [
'broadcastTo([1, 2, 3], [3, 3])',
'broadcastTo([1, 2; 3, 4], [2, 2])'
],
seealso: [
'size', 'reshape', 'broadcastSizes', 'broadcastMatrices'
]
}
15 changes: 15 additions & 0 deletions src/expression/embeddedDocs/function/matrix/broadcastMatrices.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export const broadcastMatricesDocs = {
name: 'broadcastMatrices',
category: 'Matrix',
syntax: [
'broadcastMatrices(A, B)'
],
description: 'Broadcast two matrices to compatible sizes',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This description doesn't seem quite correct, or at least not grammatically correct. If I am understanding correctly:

  • broadcastMatrices can take any positive number of arguments, not just two
  • There is just one "smallest mutually compatible size" that it broadcasts every argument to.
  • Correct me if I am wrong, but this may fail for incompatible sizes, like there is no way to broadcast a 2×3 matrix and a 3×2 matrix, correct?

So maybe something like: "Broadcast a list of matrices to their smallest compatible size (if any)"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks,

  • yes, any number of arguments, will fix.
  • I think there is only one, at least for the current definition.
  • yes, will fail for incompatible sizes.

The general terminology used is dominated by numpy with:

Broadcast any number of arrays against each other.

It could be

Broadcast any number of arrays or matrices against each other.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your final suggestion seems fine to me.

examples: [
'broadcastMatrices([1, 2, 3], [[1], [2], [3]])',
'broadcastMatrices([1, 2; 3, 4], [5, 6; 7, 8])'
],
seealso: [
'size', 'reshape', 'broadcastSizes', 'broadcastTo'
]
}
15 changes: 15 additions & 0 deletions src/expression/embeddedDocs/function/matrix/broadcastSizes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export const broadcastSizesDocs = {
name: 'broadcastSizes',
category: 'Matrix',
syntax: [
'broadcastSizes(sizeA, sizeB)'
],
description: 'Broadcast the sizes of matrices to a compatible size',
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

examples: [
'broadcastSizes([3, 1, 3], [3, 3])',
'broadcastSizes([2, 1], [2, 2])'
],
seealso: [
'size', 'reshape', 'broadcastTo', 'broadcastMatrices'
]
}
3 changes: 3 additions & 0 deletions src/factoriesAny.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ export { createOnes } from './function/matrix/ones.js'
export { createRange } from './function/matrix/range.js'
export { createReshape } from './function/matrix/reshape.js'
export { createResize } from './function/matrix/resize.js'
export { createBroadcastMatrices } from './function/matrix/broadcastMatrices.js'
export { createBroadcastSizes } from './function/matrix/broadcastSizes.js'
export { createBroadcastTo } from './function/matrix/broadcastTo.js'
export { createRotate } from './function/matrix/rotate.js'
export { createRotationMatrix } from './function/matrix/rotationMatrix.js'
export { createRow } from './function/matrix/row.js'
Expand Down
46 changes: 46 additions & 0 deletions src/function/matrix/broadcastMatrices.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { broadcastArrays } from '../../utils/array.js'
import { factory } from '../../utils/factory.js'
import { isMatrix } from '../../utils/is.js'

const name = 'broadcastMatrices'
const dependencies = ['typed']

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

Choose a reason for hiding this comment

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

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?

*
* Syntax:
*
* math.broadcastMatrices(x, y)
* math.broadcastMatrices(x, y, ...)
*
* Examples:
*
* math.broadcastMatrices([1, 2], [[3], [4]]) // returns [[[1, 2], [1, 2]], [[3, 3], [4, 4]]]
* math.broadcastMatrices([2, 3]) // returns [[2, 3]]
* math.broadcastMatrices([2, 3], [3, 1]) // returns [[2, 3], [3, 1]]
*
* See also:
*
* size, reshape, broadcastSizes, broadcastTo
*
* History:
*
* v15.1.1 created
* @param {...(Array|Matrix)} x One or more matrices or arrays
* @return {Array[Array|Matrix]} An array of matrices with the broadcasted sizes.
*/
return typed(name, {
'...Array|Matrix': collections => {
const areMatrices = collections.map(isMatrix)
if (areMatrices.includes(true)) {
const arrays = collections.map((c, i) => areMatrices[i] ? c.valueOf() : c)
const broadcastedArrays = broadcastArrays(...arrays)
const broadcastedCollections = broadcastedArrays.map((arr, i) => areMatrices[i] ? collections[i].create(arr) : arr)
return broadcastedCollections
}
return broadcastArrays(...collections)
}
})
})
44 changes: 44 additions & 0 deletions src/function/matrix/broadcastSizes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { broadcastSizes } from '../../utils/array.js'
import { factory } from '../../utils/factory.js'
import { isMatrix } from '../../utils/is.js'

const name = 'broadcastSizes'
const dependencies = ['typed']

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

Choose a reason for hiding this comment

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

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 contents 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.

* Always returns an Array containing numbers.
*
* Syntax:
*
* math.broadcastSizes(x, y)
* math.broadcastSizes(x, y, ...)
*
* Examples:
*
* math.broadcastSizes([2, 3]) // returns [2, 3]
* math.broadcastSizes([2, 3], [3]) // returns [2, 3]
* math.broadcastSizes([1, 2, 3], [1, 2, 1]) // returns [1, 2, 3]
*
* See also:
*
* size, reshape, squeeze, broadcastTo
*
* History:
*
* v15.1.1 created
* @param {...(Array|Matrix)} x One or more matrices or arrays
* @return {Array} A vector with the broadcasted size.
*/
return typed(name, {
'...Array|Matrix': collections => {
const areMatrices = collections.map(isMatrix)
if (areMatrices.includes(true)) {
const arrays = collections.map((c, i) => areMatrices[i] ? c.valueOf() : c)
return broadcastSizes(...arrays)
}
return broadcastSizes(...collections)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is precomputing areMatrices and conditioning on whether any entry is true really worth it as compared to the much simpler-looking return broadcastSizes(...collections.map(c => c.valueOf())) ? [Note that it is perfectly ok to call .valueOf on an Array, it's just a no-op.]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't checked the performance implications, I didn't consider running .valueOf() on an array.

I will do a quick check but I think you are right. Will change in code.

}
})
})
38 changes: 38 additions & 0 deletions src/function/matrix/broadcastTo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { broadcastTo } from '../../utils/array.js'
import { factory } from '../../utils/factory.js'

const name = 'broadcastTo'
const dependencies = ['typed']

export const createBroadcastTo = /* #__PURE__ */ factory(name, dependencies, ({ typed }) => {
/**
* Broadcast an array to a specified size.
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Basically the same documentation comments: needs to be clear (how to find out) what broadcasting is, and there should be mention of what happens with incompatible size. Presumably that also means that (for all three functions) there should be an example with incompatible size.

* Syntax:
*
* math.broadcastTo(x, size)
*
* Examples:
*
* math.broadcastTo([1, 2, 3], [2, 3]) // returns [[1, 2, 3], [1, 2, 3]]
* math.broadcastTo([2, 3], [2, 2]) // returns [[2, 3], [2, 3]]
*
* See also:
*
* size, reshape, squeeze, broadcastSizes
*
* History:
*
* v15.1.1 created
*
* @param {Array|Matrix} x The array or matrix to broadcast
* @param {Array|Matrix} size The target size
* @return {Array} The broadcasted array
*/
return typed(name, {
'Array, Array': broadcastTo,
'Array, Matrix': (arr, size) => broadcastTo(arr, size.toArray()),
'Matrix, Array': (M, size) => M.create(broadcastTo(M.toArray(), size)),
'Matrix, Matrix': (M1, size) => M1.create(broadcastTo(M1.toArray(), size.toArray()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Why are we using toArray() here rather than valueOf() as in the other new functions' implementation?
  • Why does the matrix implementation use M1 rather than M?
  • Consider merging the second two:
`Matrix, Array | Matrix`: (M, size) => M.create(broadcastTo(M.valueOf(), size.valueOf())`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, will review and fix in code.

})
})
2 changes: 1 addition & 1 deletion src/utils/array.js
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,7 @@ export function broadcastArrays (...arrays) {
throw new Error('Insufficient number of arguments in function broadcastArrays')
}
if (arrays.length === 1) {
return arrays[0]
return arrays
}
const sizes = arrays.map(function (array) { return arraySize(array) })
const broadcastedSize = broadcastSizes(...sizes)
Expand Down
35 changes: 35 additions & 0 deletions test/unit-tests/function/matrix/broadcastMatrices.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import assert from 'assert'
import math from '../../../../src/defaultInstance.js'

describe('broadcastMatrices', function () {
const matrix = math.matrix
const broadcastMatrices = math.broadcastMatrices
const A = [[1], [2], [3]]
const B = [[10, 20, 30]]
const C = [100]
const broadcastedA = [[1, 1, 1], [2, 2, 2], [3, 3, 3]]
const broadcastedB = [[10, 20, 30], [10, 20, 30], [10, 20, 30]]
const broadcastedC = [[100, 100, 100], [100, 100, 100], [100, 100, 100]]

it('should broadcast matrices', function () {
assert.deepStrictEqual(broadcastMatrices(matrix(A), matrix(B)), [matrix(broadcastedA), matrix(broadcastedB)])
assert.deepStrictEqual(broadcastMatrices(matrix(A), matrix(C)), [matrix(A), matrix([[100], [100], [100]])])
assert.deepStrictEqual(broadcastMatrices(matrix(B), matrix(A)), [matrix(broadcastedB), matrix(broadcastedA)])
assert.deepStrictEqual(broadcastMatrices(matrix(A), matrix(B), matrix(C)), [matrix(broadcastedA), matrix(broadcastedB), matrix(broadcastedC)])
})

it('should broadcast arrays', function () {
assert.deepStrictEqual(broadcastMatrices(A, B), [broadcastedA, broadcastedB])
assert.deepStrictEqual(broadcastMatrices(B, A), [broadcastedB, broadcastedA])
assert.deepStrictEqual(broadcastMatrices(A, B, C), [broadcastedA, broadcastedB, broadcastedC])
assert.deepStrictEqual(broadcastMatrices(A), [A])
assert.deepStrictEqual(broadcastMatrices(B, C, A), [broadcastedB, broadcastedC, broadcastedA])
})

it('should throw an error if sizes are not compatible', function () {
assert.throws(function () { broadcastMatrices(matrix([[1, 2], [3, 4]]), matrix([[1, 2, 3]])) }, /Error: shape mismatch: /)
assert.throws(function () { broadcastMatrices([[1, 2], [3, 4]], matrix([[1, 2, 3]])) }, /Error: shape mismatch: /)
assert.throws(function () { broadcastMatrices(matrix([[1, 2], [3, 4]]), [[1, 2, 3]]) }, /Error: shape mismatch: /)
assert.throws(function () { broadcastMatrices([[1, 2], [3, 4]], [[1, 2, 3]]) }, /Error: shape mismatch: /)
})
})
27 changes: 27 additions & 0 deletions test/unit-tests/function/matrix/broadcastSizes.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import assert from 'assert'
import math from '../../../../src/defaultInstance.js'

describe('broadcastSizes', function () {
const broadcastSizes = math.broadcastSizes
const matrix = math.matrix

it('should broadcast sizes', function () {
assert.deepStrictEqual(broadcastSizes([2, 3]), [2, 3])
assert.deepStrictEqual(broadcastSizes([3, 3], [3, 1]), [3, 3])
assert.deepStrictEqual(broadcastSizes([2, 1], [1, 3]), [2, 3])
assert.deepStrictEqual(broadcastSizes([5, 4, 3], [1, 4, 1]), [5, 4, 3])
assert.deepStrictEqual(broadcastSizes([3], [2, 3]), [2, 3])
assert.deepStrictEqual(broadcastSizes([1, 3], [2, 1]), [2, 3])
})

it('should throw an error if sizes are not compatible', function () {
assert.throws(function () { broadcastSizes([2, 3], [3, 2]) }, /Error: shape mismatch: /)
assert.throws(function () { broadcastSizes([2, 3], [2, 3, 4]) }, /Error: shape mismatch: /)
})

it('should broadcast sizes of mixed arrays and matrices', function () {
assert.deepStrictEqual(broadcastSizes([3, 3], matrix([3, 1])), [3, 3])
assert.deepStrictEqual(broadcastSizes(matrix([2, 1]), [1, 3]), [2, 3])
assert.deepStrictEqual(broadcastSizes(matrix([5, 4, 3]), matrix([1, 4, 1])), [5, 4, 3])
})
})
29 changes: 29 additions & 0 deletions test/unit-tests/function/matrix/broadcastTo.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import assert from 'assert'
import math from '../../../../src/defaultInstance.js'

describe('broadcastTo', function () {
const broadcastTo = math.broadcastTo
const matrix = math.matrix

it('should broadcast arrays to a given size', function () {
assert.deepStrictEqual(broadcastTo([1, 2, 3], [2, 3]), [[1, 2, 3], [1, 2, 3]])
assert.deepStrictEqual(broadcastTo([2, 3], [2, 2]), [[2, 3], [2, 3]])
})

it('should broadcast matrices to a given size', function () {
assert.deepStrictEqual(broadcastTo(matrix([1, 2, 3]), [2, 3]), matrix([[1, 2, 3], [1, 2, 3]]))
assert.deepStrictEqual(broadcastTo(matrix([2, 3]), [2, 2]), matrix([[2, 3], [2, 3]]))
})

it('should broadcast mixed arrays and matrices to a given size', function () {
assert.deepStrictEqual(broadcastTo([1, 2, 3], matrix([2, 3])), [[1, 2, 3], [1, 2, 3]])
assert.deepStrictEqual(broadcastTo(matrix([2, 3]), [2, 2]), matrix([[2, 3], [2, 3]]))
assert.deepStrictEqual(broadcastTo(matrix([1, 2, 3]), matrix([2, 3])), matrix([[1, 2, 3], [1, 2, 3]]))
assert.deepStrictEqual(broadcastTo([2, 3], matrix([2, 2])), [[2, 3], [2, 3]])
})

it('should throw an error if sizes are not compatible', function () {
assert.throws(function () { broadcastTo([1, 2], [2, 3]) }, /Error: shape mismatch: /)
assert.throws(function () { broadcastTo(matrix([1, 2]), [2, 3]) }, /Error: shape mismatch: /)
})
})
6 changes: 3 additions & 3 deletions test/unit-tests/utils/array.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -702,9 +702,9 @@ describe('util.array', function () {
})

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

Choose a reason for hiding this comment

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

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.

assert.deepStrictEqual(broadcastArrays([1, 2]), [1, 2], [3, 4])
assert.deepStrictEqual(broadcastArrays([[3], [4]]), [[3], [4]])
assert.deepStrictEqual(broadcastArrays([[5, 6]]), [[5, 6]])
assert.deepStrictEqual(broadcastArrays([1, 2])[0], [1, 2])
assert.deepStrictEqual(broadcastArrays([[3], [4]])[0], [[3], [4]])
assert.deepStrictEqual(broadcastArrays([[5, 6]])[0], [[5, 6]])
})

it('should throw an arryor when the broadcasting rules don\'t apply', function () {
Expand Down
Loading