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

Refactor: replace numeric with mathjs #225

Closed
wants to merge 6 commits into from

Conversation

idiotWu
Copy link
Contributor

@idiotWu idiotWu commented Jun 15, 2021

Resolves #211.

This PR replaced numeric with mathjs to fix the CSP error as mentioned in #211.
I'm not able to run the test script because files under www/data/src/P_01/ are missing, but the demo pages seem to be working.

As a side effect, the bundle size increased from 2.1M to 3.2M (both minified).

src/util_regression.mjs Outdated Show resolved Hide resolved
src/util_regression.mjs Outdated Show resolved Hide resolved
@@ -368,7 +368,8 @@
self.webgazer.util.KalmanFilter.prototype.update = function(z) {

// Here, we define all the different matrix operations we will need
var add = numeric.add, sub = numeric.sub, inv = numeric.inv, identity = numeric.identity;
// FIXME: mathjs is not exported to worker global scope
var add = mathjs.add, sub = mathjs.subtract, inv = mathjs.inv, identity = mathjs.identity;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this works since neither the numeric nor the mathjs variable is exported to the worker global scope. (as commented in #163 (comment))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, we need to fix that separately.

solution = (ss.length === ss[0].length ? (numeric.LUsolve(numeric.LU(ss,true),bb)) : (webgazer.mat.QRDecomposition(ss,bb)));
solution = ss.length === ss[0].length ?
mathjs.squeeze(mathjs.lusolve(ss, bb)) : // lusolve returns a column vector, so squeeze it
webgazer.mat.QRDecomposition(ss,bb);
Copy link
Collaborator

Choose a reason for hiding this comment

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

math.js has a qr decomposition too, right?

Copy link
Contributor Author

@idiotWu idiotWu Jun 15, 2021

Choose a reason for hiding this comment

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

Right, but the two functions are different: mathjs.qr(A) only decomposes matrix A, while mat.QRDecomposition(A, b) solves the linear system Ax = b

Copy link
Contributor Author

@idiotWu idiotWu Jun 16, 2021

Choose a reason for hiding this comment

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

The equivalent qrsolve() function can be implemented as:

function qrsolve(A, b) {
  const { Q, R } = mathjs.qr(A);
  const dim1 = A[0].length;
  
  return math.multiply(
    mathjs.inv(R.slice(0, dim1)),
    mathjs.transpose(Q).slice(0, dim1),
    b,
  );
}

However, I don't think it will be faster than the current one.

Copy link
Contributor Author

@idiotWu idiotWu Jun 16, 2021

Choose a reason for hiding this comment

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

Those implements in mat.mjs seem to be much faster than mathjs in my tests:

image

Test Code

Copy link
Collaborator

Choose a reason for hiding this comment

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

WOW. Just wow. Add a comment so we don't accidentally use them in the future. We should verify the matmul and other functions in math.js are actually faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mat.mul() is also faster than mathjs.multiply() after 1M runs:

image

Maybe mathjs is wasting too much time constructing matrix instances 😞

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather disappointing. Are there any other good candidates that may work better than math.js. Surely something has to be faster than numeric, maybe something that even leveraged WASM?

Copy link
Contributor Author

@idiotWu idiotWu Jun 16, 2021

Choose a reason for hiding this comment

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

In my tests, mat.* >>> numeric >> ml-matrix ~ math.js. Libraries like math.js add extra checks to ensure operations can be performed, which causes a decline in performance. Maybe the best solution is to implement all the required matrix operations ourselves.

Copy link
Collaborator

Choose a reason for hiding this comment

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

mat.mul() is also faster than mathjs.multiply() after 1M runs:

image

Maybe mathjs is wasting too much time constructing matrix instances 😞

Looked into the code and apparently the matrix instances are just a wrapper around associative arrays so that shouldn't slow it down. I did see that it did have serious speed issues though. It's designed to be flexible and extensible, not fast.

@Skylion007
Copy link
Collaborator

Yeah, I found that Math.js wraps normal JS arrays. Hence why it's slow. It also doesn't do a lot of optimizations. It seems to be more focused on correctness and extensibility than speed sadly.

Instead of reimplementing all these ourselves, I would like to see if there is a well-supported LinAlg library we can leverage. At the very least, we are using arrays of arrays in JS which are really slow, so anything that doesn't use those should perform faster.

@idiotWu It may be overkill but how does the benchmarking for Eigen.JS look? https://github.com/BertrandBev/eigen-js

@Skylion007
Copy link
Collaborator

Here is there official benchmarking: https://bertrandbev.github.io/eigen-js/#/benchmark

@idiotWu
Copy link
Contributor Author

idiotWu commented Jun 16, 2021

@Skylion007

At the very least, we are using arrays of arrays in JS which are really slow, so anything that doesn't use those should perform faster.

Well, JS native arrays are actually fast on modern browsers. I thought TypedArray based implement would be faster, but in my tests ml-matrix (the only TypedArray based linalg lib I've found so far) does not seem to be much faster than math.js. What's more, a total refactoring is necessary if we want to use TypedArray.

Eigen.js seems to be as slow as math.js:
image

@istvan73
Copy link

@idiotWu

I just dropped in to say a big THANK YOU!
You've solved the issue which held me back from making my chrome extension. I am super grateful, and going to mention your code as well when the extension will be out.

Istvan

@koll93
Copy link
Contributor

koll93 commented Sep 18, 2022

@idiotWu Thanks.
I needed to use webgaizer in google extension with manifest v3 but with new model with landmarks. I used @idiotWu's idea for the latest revision of the webgaizer. https://github.com/koll93/WebGazer/tree/refactor-remove-numeric-landmarks

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

Successfully merging this pull request may close these issues.

request to move off numeric
4 participants