feat: add new matrix addition method and improve performance of mmul and kroneckerProduct method

[jobo322]

No description provided.

[jobo322]

Looks that the use of callbacks for matrix operations is not the most performant approach.

I propose to use directly the data in this.element

Cardinality	Operation	Original (ms)	New (ms)	Speedup	Improvement
664, 665	mmul	76.42	2.80	27.29x	2728.9%
664, 665	kroneckerProduct	108.70	31.53	3.45x	344.8%
180, 133	mmul	6.27	0.70	8.92x	892.3%
180, 133	kroneckerProduct	5.83	2.00	2.91x	291.1%
2648, 2584	mmul	1175.73	30.10	39.06x	3906.4%
2648, 2584	kroneckerProduct	1968.29	753.71	2.61x	261.1%

[targos]

Is this result based on the existing benchmark in the repo? Otherwise, can you please add the benchmark?

[codecov]

Codecov Report

Attention: Patch coverage is 99.54545% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.75%. Comparing base (9361aa3) to head (5a7b46e).

Files with missing lines	Patch %	Lines
src/index.js	98.95%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #14       +/-   ##
===========================================
+ Coverage   55.60%   71.75%   +16.14%     
===========================================
  Files           5        5               
  Lines        1453     1416       -37     
  Branches       46       72       +26     
===========================================
+ Hits          808     1016      +208     
+ Misses        641      400      -241     
+ Partials        4        0        -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[targos]

I'm sorry if this sounds harsh, but homemade benchmark scripts have little to no validity. There are libraries that are made to create benchmarks correctly and take care of everything including statistical stuff. Can you explain why you didn't use one of them?

[targos]

Also, your code in src/index.js introduces variables names and patterns that are really not obvious (csr, csc, coo, format). It should at least have some comments and jsdoc, and also external links for references.

And again, please do not use Spanish in code or comments.

[targos]

You can try to use mitata: https://github.com/evanwashere/mitata
It even has a guide to write good benchmarks https://github.com/evanwashere/mitata#writing-good-benchmarks

[jobo322]

Looks that the use of callbacks for matrix operations is not the most performant approach.

I propose to use directly the data in this.element

Cardinality Operation Original (ms) New (ms) Speedup Improvement
664, 665 mmul 76.42 2.80 27.29x 2728.9%
664, 665 kroneckerProduct 108.70 31.53 3.45x 344.8%
180, 133 mmul 6.27 0.70 8.92x 892.3%
180, 133 kroneckerProduct 5.83 2.00 2.91x 291.1%
2648, 2584 mmul 1175.73 30.10 39.06x 3906.4%
2648, 2584 kroneckerProduct 1968.29 753.71 2.61x 261.1%

This comparison was with an assumption that is not always true so thy need to reorder the data reduce the current performance but it is still better

[jobo322]

I'm sorry if this sounds harsh, but homemade benchmark scripts have little to no validity. There are libraries that are made to create benchmarks correctly and take care of everything including statistical stuff. Can you explain why you didn't use one of them?

Yes you are just right, I also noticed some inconsistencies, I did try with benchmark package before read your comments. I hope it looks much better now

[targos]

Please pay attention. You force-pushed on the merge that I took time to make. I merged again, so please don't overwrite it again.

[jobo322]

size: 4 - 100 square matrices, density = 1 % with at least 1 element.

size: 4 - 76 square matrices, density = 2% with at least 1 element.

size: 4 - 76 square matrices, density = 3% with at least 1 element.

[jobo322]

Please pay attention. You force-pushed on the merge that I took time to make. I merged again, so please don't overwrite it again.

I apologise, if you give me the rights to create a PR directly into mljs I could do it. I did force it again, just to include a missing change.

[targos]

This has nothing to do with permissions. Apologizing doesn't make it more ok to cancel my work again

[lpatiny]

@jobo322 Please could you add the matrix density of the ibuprofen simulation

[targos]

Please don't merge this until I return from holiday. (and try to merge back with my work (ac26a10), which was force pushed out of this branch)

[jobo322]

@lpatiny in the particular of ibuprofen, the size of the matrices increase with decreasing of the density, in those cases, the new mmul method is still faster

[jobo322]

There are three regimens of matrix multiplication of sparse matrices based on the way the data is stored. The three method proposed are:

1. __mmulSmall get only the nonZeros of the right matrix, it is faster when there are few non-zeros, usually cardinality below 45
2. __mmulLowDensity get the nonZeros of both matrices before iterate. it is faster when:
   • The number of rows of the right matrix is greater than 100 and
   • The cardinality is less than 100 elements.
3. __mmulMediumDensity get the nonZeros of both matrices, sort and format the non-zeros elements of the right matrix, it is faster and is the default method to multiply matrices.

the Method mmul is a hybrid of those three methods with faster behavior than the current implementation:

size: 4 - 36 square matrices, density = 1% with at least 1 element.

size: 4 - 36 square matrices, density = 2% with at least 1 element.

size: 4 - 36 square matrices, density = 3% with at least 1 element.

[targos]

@jobo322 Is this ready for a full review?

[jobo322]

@jobo322 Is this ready for a full review?

yes, I did implement your suggestion about the documentation and use the specialized matrix functions as external functions