Arguments to matrix operations appear to be modified by side effect

[gwhitney]

          fair enough ....

step 1: I create a scope,

        if (decaySliceApiCall.isSuccess) {
            const scope = {}
            channels.map((channel) => {
                scope[`Ch${channel}`] = {}
                decays.map((decay) => {
                    scope[`Ch${channel}`][`D${decay}`] = decaySliceApiCall.data.data.z.channels[`${channel}`].decays[`${decay}`]
                })
            })
            setScope(scope)

the scope looks like:

e.g Ch0.D10 is a 2D array of numbers

if I use the expression Ch0.D40./Ch2.D40 ....

let expression = "Ch0.D40./Ch2.D40"
        if (scope) {
            let localScope = { ...scope }
            try {
                //math.evaluate('fImage=' + expression, localScope).toArray()

                const node = math.parse(expression)
                const code = node.compile()
                console.log(code.evaluate(localScope))
                
            } catch (error) {
                console.error(error);
            }
        }

the outpur is an Error

interestingly,

if i change the expression to:

const node = math.parse('Ch2.D40')

the the output is fine:

hope this helps.

Originally posted by @pramod-bls in #2596 (comment)

[gwhitney]

Hmm, I am having trouble reproducing, @pramod-bls. In the version of your code where it is erroring, please just before you call node.compile could you put the line:

node.traverse(n => console.log(n.toJSON()))

and just before the call to code.evaluate that errors, could you console.log(code.evaluate) ? And then run your code and post the output of those added lines here?

[gwhitney]

Oh, and can you also do a console.log(math.evaluate('version')) and let us know what that says just for certainty about what version you are running? Just to provide some color here, I am very confused as to how your evaluate that is erroring is getting into the guts of the (matrix) multiply function, when ostensibly the expression you're evaluating has no multiplications in it...

[pramod-bls]

The version is: 11.8.2 Thanks, Pramod From: Glen Whitney ***@***.***> Date: Tuesday, July 4, 2023 at 6:12 PM To: josdejong/mathjs ***@***.***> Cc: Pramod Butte ***@***.***>, Mention ***@***.***> Subject: Re: [josdejong/mathjs] Evaluation/parsing problem in Electron (Issue #2990) Oh, and can you also do a console.log(math.evaluate('version')) and let us know what that says just for certainty about what version you are running? Just to provide some color here, I am very confused as to how your evaluate that is erroring is getting into the guts of the (matrix) multiply function, when ostensibly the expression you're evaluating has no multiplications in it... — Reply to this email directly, view it on GitHub<#2990 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A6G4I7L4XSS4JVID4QB2FETXOS5Q7ANCNFSM6AAAAAAZ6C76YA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[gwhitney]

Great, thanks. Did you also see the request for node.traverse(n => console.log(n.toJSON())) and console.log(code.evaluate) ?

[gwhitney]

Hmm, no output in that message, at least not that made it to github. Maybe you can use the web interface and put it in a comment?

[gwhitney]

Yeah nothing there either. If you are emailing with attached images (for example), it appears the images are being stripped by the Github email processor.

[pramod-bls]

Also the output of the

node.traverse(n => console.log(n.toJSON())) & console.log(code.evaluate)

let me know if this helps

[gwhitney]

Yes, that's very helpful. The error is in a different place now that corresponds to the expression you are showing. I'll keep looking.

[pramod-bls]

Yes, that's very helpful. The error is in a different place now that corresponds to the expression you are showing. I'll keep looking.

Thank you!

[gwhitney]

OK, I have reproduced the error without electron. The difficulty is that mathjs is modifying the numerator 2D array in a dotDivide, which is totally counterintuitive and frankly seems like a significant bug in mathjs; simply executing a dotDivide should not have any side effect on its arguments. It seems that it has come up for you because whatever the specifics are of how your 2D arrays are being created, they end up read-only, so the modification fails. Here is the minimal reproducing code I have come up with:

import { parse } from 'mathjs'
const scope = {}
scope.Ch0 = {}
scope.Ch0.D40 = [[4,6,8,10],[3,6,9,12]]
Object.freeze(scope.Ch0.D40)
scope.Ch2 = {}
scope.Ch2.D40 = [[2,3,4,5],[3,3,3,3]]
const node = parse('Ch0.D40./Ch2.D40')
const code = node.compile()
console.log(code.evaluate(scope))

which in Node produces the output

file:///home/glen/code/mathjs/lib/esm/type/matrix/DenseMatrix.js:933
        data[i] = preprocess(elem);
                ^

TypeError: Cannot assign to read only property '0' of object '[object Array]'
    at preprocess (file:///home/glen/code/mathjs/lib/esm/type/matrix/DenseMatrix.js:933:17)
    at new DenseMatrix (file:///home/glen/code/mathjs/lib/esm/type/matrix/DenseMatrix.js:49:20)
    at _create (file:///home/glen/code/mathjs/lib/esm/type/matrix/function/matrix.js:75:14)
    at Array (file:///home/glen/code/mathjs/lib/esm/type/matrix/function/matrix.js:55:14)
    at matrix (/home/glen/code/mathjs/node_modules/typed-function/lib/umd/typed-function.js:1802:22)
    at Array, Array (file:///home/glen/code/mathjs/lib/esm/type/matrix/utils/matrixAlgorithmSuite.js:49:61)
    at generic (/home/glen/code/mathjs/node_modules/typed-function/lib/umd/typed-function.js:1782:29)
    at dotDivide (/home/glen/code/mathjs/node_modules/typed-function/lib/umd/typed-function.js:1817:24)
    at evalOperatorNode (file:///home/glen/code/mathjs/lib/esm/expression/node/OperatorNode.js:298:18)
    at Object.evaluate (file:///home/glen/code/mathjs/lib/esm/expression/node/Node.js:56:16)

Node.js v19.1.0

I will be frank that it may take some time to track down/fix this apparently pretty deep-seated error. (I will rename this issue and escalate it to the extent possible.)

I guess it's never come up before that people have tried to use mathjs on read-only inputs? That's a bit surprising, and also a bit surprising that nobody has noticed them modifying their inputs by side-effect. Apologies that there is this underlying issue, but thanks so much for surfacing it for us! We'll try to delve into it without undue delay.

[pramod-bls]

Thank you Glen, I did suspect that and hence I even changed to code to have a local modifiable scope like so,

var localScope = { ...scope }

but I guess it is still treated as read-only.

Best,

Pramod

[josdejong]

Thanks for bringing this up and debugging the issue guys. It looks like this preprocess function inside DenseMatrix is the cause here: it mutates the input. I can reproduce the issue with the following code:

const data = [[1, 2]]
Object.freeze(data)

const matrix = new DenseMatrix(data) 
// TypeError: Cannot assign to read only property '0' of object '[object Array]'

This function preprocess replaces nested Matrix instances with Array. This should be done in an immutable way, we need to fix that. This is an edge case though, in general you have Arrays with nested Arrays, and in that case the function preprocess only replaces the nested arrays with themselves, which has no effect. That is probably the reason that this issue was never spotted so far and didn't lead to issues in practice.

I'm halfway creating a fix for this.

[josdejong]

I've created a PR to address this issue, see #2991.

@gwhitney or @pramod-bls can you have a look at it?

[pramod-bls]

apologies for late reply but i am unable to test the PR,

when I try to install the PR using:

npm install git+https://github.com/josdejong/mathjs.git#fix/2990_densematrix_mutating_input --save

I get a error:

no extension
              Field 'browser' doesn't contain a valid alias configuration
              /Users/buttep/projects/electron_frontend_for_fastapi_docker_server/node_modules/mathjs is not a file
            .js
              Field 'browser' doesn't contain a valid alias configuration
              /Users/buttep/projects/electron_frontend_for_fastapi_docker_server/node_modules/mathjs.js doesn't exist
        existing directory /Users/buttep/projects/electron_frontend_for_fastapi_docker_server/node_modules/mathjs
          using description file: /Users/buttep/projects/electron_frontend_for_fastapi_docker_server/node_modules/mathjs/package.json (relative path: .)
            using exports field: ./lib/esm/index.js
              using description file: /Users/buttep/projects/electron_frontend_for_fastapi_docker_server/node_modules/mathjs/package.json (relative path: ./lib/esm/index.js)
                no extension
                  Field 'browser' doesn't contain a valid alias configuration
                  /Users/buttep/projects/electron_frontend_for_fastapi_docker_server/node_modules/mathjs/lib/esm/index.js doesn't exist
                .js
                  Field 'browser' doesn't contain a valid alias configuration
                  /Users/buttep/projects/electron_frontend_for_fastapi_docker_server/node_modules/mathjs/lib/esm/index.js.js doesn't exist
                as directory
                  /Users/buttep/projects/electron_frontend_for_fastapi_docker_server/node_modules/mathjs/lib/esm/index.js doesn't exist

Any suggestions?

[josdejong]

@pramod-bls this may be because you cannot use the source code as-is: you have to run the build-step before it is usable. Can you clone the project, checkout the feature branch, run npm install && npm run build and then add the library to your project?

[pramod-bls]

I can confirm that this resolves the issue.

[josdejong]

Thanks for verifying this @pramod-bls . We'll merge and publish the fix soon.

[josdejong]

The fix is published now in v11.10.0