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

use sparse multiplication for sparse arrays time BitMatrix and BitVector and wrappers #39557

Merged
merged 2 commits into from
Feb 8, 2021

Conversation

abraunst
Copy link
Contributor

@abraunst abraunst commented Feb 7, 2021

My attempt to fix #39474
Not really sure about the types renaming, can be rolled back or changed of course.

@abraunst
Copy link
Contributor Author

abraunst commented Feb 7, 2021

I can add some tests if this is deemed the way forward.

@abraunst abraunst marked this pull request as ready for review February 7, 2021 11:48
@kshyatt kshyatt added sparse Sparse arrays linear algebra Linear algebra labels Feb 7, 2021
@ViralBShah
Copy link
Member

@dkarrasch Can you also take a look?

@ViralBShah
Copy link
Member

Let's add the tests. I'm not sure - but is nanosoldier functional at the moment?

@abraunst
Copy link
Contributor Author

abraunst commented Feb 8, 2021

Let's add the tests. I'm not sure - but is nanosoldier functional at the moment?

About the tests... how can we target the fact that the multiplication is the sparse one?
Or maybe we just need benchmarking?

@dkarrasch
Copy link
Member

About the tests... how can we target the fact that the multiplication is the sparse one?

I think you can do something like

m = @which mul!(C, A, B, true, false)
@test m.module == SparseArrays

Or maybe we just need benchmarking?

It would be interesting to see whether we have tests that cover the BitVector case. But to make sure we don't regress from where we currently are, let's run Nanosoldier.

@nanosoldier runbenchmarks("sparse", vs=":master")

@abraunst
Copy link
Contributor Author

abraunst commented Feb 8, 2021

@nanosoldier runbenchmarks("sparse", vs=":master")

nanosoldier seems MIA :(

@dkarrasch
Copy link
Member

Sometimes it's actually running but fails to leave a footprint. Unfortunately, I don't know where to look for whether it's running. I'll wait two or three hours and then try again.

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

LGTM. I think this is carefully written to include "BitVecOrMat", but otherwise not change current behavior. I'd not expect to learn much from a Nanosoldiers run.

@dkarrasch dkarrasch added the performance Must go faster label Feb 8, 2021
@ViralBShah
Copy link
Member

That's what I thought as well - I think it should be good to merge.

I think nanosoldier is out of commission (and physical access to MIT is needed to fix it - which is not likely to be approved anytime soon).

@ViralBShah
Copy link
Member

@dkarrasch Please merge if good.

@dkarrasch dkarrasch merged commit e3753e1 into JuliaLang:master Feb 8, 2021
@abraunst abraunst deleted the bitarraytimessparse branch February 8, 2021 17:48
@ViralBShah ViralBShah added the backport 1.6 Change should be backported to release-1.6 label Feb 8, 2021
KristofferC pushed a commit that referenced this pull request Feb 11, 2021
@KristofferC KristofferC mentioned this pull request Feb 11, 2021
52 tasks
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Mar 14, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra performance Must go faster sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SparseMatrixCSC times BitVector falls back to dense multiplication
5 participants