Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify not working with matrix multiplication? #2614

Open
filonik opened this issue Jul 1, 2022 · 3 comments
Open

Simplify not working with matrix multiplication? #2614

filonik opened this issue Jul 1, 2022 · 3 comments

Comments

@filonik
Copy link

filonik commented Jul 1, 2022

Shouldn't the following produce the same result?

import * as math from 'mathjs'

let scope = {
  A: math.matrix([
    [1, 2],
    [3, 4],
  ]),
  v: math.matrix([5, 6]),
}

const r0 = math.evaluate('A * v', scope)
console.log('Evaluate:', r0) // Evaluate: DenseMatrix {_data: [17, 39]}

//const A = new math.SymbolNode('A')
//const v = new math.SymbolNode('v')
//const expr = new math.OperatorNode('*', 'multiply', [A, v])
const expr = math.parse('A * v')

const r1 = math.simplify(expr, scope)
console.log('Simplify:', r1) // Simplify: ConstantNode {value: 0}

When I run the code, evaluate produces the expected output ([17, 39]), but simplify does not (it instead produces 0 which doesn't seem right).

@josdejong
Copy link
Owner

Thanks for reporting, that sounds like a bug, 0 is indeed a wrong result for that expression.

Anyone able to help debugging/fixing this issue in simplify? Help would be welcome.

@soleilcot
Copy link

I took a look at this as it appears the issue still occurs. The reason for 0 being returned lies in the simplifyCore.js logic that executes as part of the simplification rule set:

if (node.op === '*') {
if (isConstantNode(a0)) {
if (isZero(a0.value)) {
return node0
} else if (equal(a0.value, 1)) {
return a1
}
}
if (isConstantNode(a1)) {
if (isZero(a1.value)) {
return node0
} else if (equal(a1.value, 1)) {
return a0
}
if (isCommutative(node, context)) {
return new OperatorNode(node.op, node.fn, [a1, a0], node.implicit) // constants on left
}
}
return new OperatorNode(node.op, node.fn, [a0, a1], node.implicit)

In this logic, both the matrix nodes are considered ConstantNode and isZero is also truthy for the DenseMatrix node values. The reason isZero is truthy in this case is because the return for a matrix argument to isZero is itself a matrix of booleans resulting from an element wise execution of isZero. In this context, simplifyCore simplifies the whole expression to 0.

Skipping that one condition wouldn't be enough though. It seems that most the simplification rules are for use in a one-dimensional algebraic context in which there can be unknowns and are thus not needed for constant matrix simplification. Instead it seems to me that we would need a new, separate set of logic specifically for matrix simplification and evaluation.

@josdejong
Copy link
Owner

Thanks for debugging this @soleilcot , this makes sense indeed.

So as a first step we may need to add a check on whether the value we're operating on is a numeric value and not a Matrix or something else I think? Like adding a check isNumeric(a0).value?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants