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

SUNMatScaleAdd_Sparse has time complexity O(M*N) #253

Closed
phannebohm opened this issue Feb 20, 2023 · 3 comments
Closed

SUNMatScaleAdd_Sparse has time complexity O(M*N) #253

phannebohm opened this issue Feb 20, 2023 · 3 comments

Comments

@phannebohm
Copy link
Contributor

As the title says, adding two sparse matrices scales badly. I believe the problem are loops like the sparsity pattern overlap check here:

/* determine if A already contains the sparsity pattern of B */
newvals = 0;
for (j=0; j<N; j++) {
/* clear work array */
for (i=0; i<M; i++) w[i] = 0;

Just filling the work array w with zeros takes M*N operations. This happens in a few places inside this function with w and x. If I replace the for loops with calls to memset I save a significant amount of computation time (maybe 75% on my machine, don't trust that number though). However it still scales badly. I would expect addition to scale like O(NNZ) instead.

On a side note, for my particular use case (OpenModelica/OpenModelica#10178) I can take a modified version of SUNMatScaleAddI_Sparse, I call it SUNMatScaleIAdd_Sparse where the result is $A + c I$. This scales linearly as expected. Perhaps this function could be useful for other people.

@balos1
Copy link
Member

balos1 commented Mar 2, 2023

Thanks for noting this. Indeed this is because the current implementation support a very general case. Perhaps we can add an option (controlled with a Set function) to use less general, but faster implementations of ScaleAdd and ScaleAddI.

@phannebohm
Copy link
Contributor Author

I think I have a general solution that avoids these scaling issues, at least for ScaleAddI. I can open a PR for it and see if it can also be ported to ScaleAdd.

phannebohm added a commit to phannebohm/sundials that referenced this issue Mar 2, 2023
Modify element traversal in SUNMatScaleAddI_Sparse to
scale O(NNZ) instead of O(M*N)

Related issue LLNL#253

Signed-off-by: phannebohm <phannebohm@fh-bielefeld.de>
@phannebohm phannebohm changed the title SUNMatScaleAdd_Sparse scales like O(M*N) SUNMatScaleAdd_Sparse hase time complexity O(M*N) Apr 5, 2023
@phannebohm phannebohm changed the title SUNMatScaleAdd_Sparse hase time complexity O(M*N) SUNMatScaleAdd_Sparse has time complexity O(M*N) Apr 5, 2023
gardner48 pushed a commit that referenced this issue Sep 18, 2023
Fixes part of #253 by avoiding nested loops that take `O(M*N)` operations.

Also fixes #256.

---------

Signed-off-by: phannebohm <phannebohm@fh-bielefeld.de>
Co-authored-by: Cody Balos <balos1@llnl.gov>
@balos1
Copy link
Member

balos1 commented Oct 20, 2023

Closed by #257.

@balos1 balos1 closed this as completed Oct 20, 2023
gardner48 pushed a commit that referenced this issue Dec 18, 2023
Fixes part of #253 by avoiding nested loops that take `O(M*N)` operations.

Also fixes #256.

---------

Signed-off-by: phannebohm <phannebohm@fh-bielefeld.de>
Co-authored-by: Cody Balos <balos1@llnl.gov>
balos1 added a commit that referenced this issue Dec 18, 2023
Fixes part of #253 by avoiding nested loops that take `O(M*N)` operations.

Also fixes #256.

---------

Signed-off-by: phannebohm <phannebohm@fh-bielefeld.de>
Co-authored-by: Cody Balos <balos1@llnl.gov>
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

2 participants