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

MatrixHandler::matrixInfNorm should not assume that the input matrix is in the csr format #180

Closed
superwhiskers opened this issue Jul 30, 2024 · 3 comments · Fixed by #190
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@superwhiskers
Copy link
Collaborator

currently, the matrixInfNorm method of MatrixHandler and its corresponding implementation classes, MatrixHandlerCpu et al. (introduced in #102) incorrectly assume that the input matrix will always be in the csr format. this conflicts with the type signature, which indicates the matrix just has to be of the Sparse class.

this is bad for multiple reasons:

  • users may incorrectly assume that it permitting the input matrix to be any Sparse matrix means it works for any such matrix
  • this will silently fail on Coo matrices, as all of their rows/columns/values are large enough to accept being indexed that way
    • the error manifests as a performance problem that can be traced back to MatrixHandler::matrixInfNorm
    • this also results in incorrect data with a nonobvious source
  • this will cause memory access errors when used on Csc matrices

this should be resolved by

  • adding a disclaimer in the documentation about this (immediately)
  • modifying the method by
    • not accepting just any matrix and specializing it to Csr or
    • generalizing it to work over different matrix formats
@superwhiskers superwhiskers added bug Something isn't working documentation Improvements or additions to documentation labels Jul 30, 2024
@pelesh pelesh self-assigned this Jul 30, 2024
@pelesh
Copy link
Collaborator

pelesh commented Jul 30, 2024

Thanks for documenting this @superwhiskers!

@pelesh
Copy link
Collaborator

pelesh commented Sep 18, 2024

It should probably be specialized to Csr matrices. Support for other formats could be added later by function overloading, if needed.

@pelesh
Copy link
Collaborator

pelesh commented Sep 24, 2024

Best thing we can do in the short term is tp check if the matrix is CSR and return error, if it is not. It is unlikely that matvec and matrixInfNorm methods of MatrixHandler class will support any other format in the near future.

The quick fix is in #190.

The challenge here is that solver implementation should be agnostic of the matrix type. Because of that, MatrixHandler methods signatures should accept generic matrix::Sparse objects. However, implementation of MatrixHandler methods is specific to the matrix format.

CC @kswirydo @stonecoldhughes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants