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

feat: add ArrowheadMatrix #21

Merged
merged 18 commits into from
Oct 11, 2024
Merged

feat: add ArrowheadMatrix #21

merged 18 commits into from
Oct 11, 2024

Conversation

Nimrais
Copy link
Contributor

@Nimrais Nimrais commented Oct 8, 2024

This PR introduces an efficient implementation of ArrowheadMatrix, a specialized matrix.

ArrowheadMatrix : Implements a memory-efficient representation of arrowhead matrices https://en.wikipedia.org/wiki/Arrowhead_matrix.

Basic Operations: Supports essential matrix operations like size, conversion to dense matrix, and multiplication with vectors.

Linear System Solving: Implements an efficient algorithm for solving linear systems involving arrowhead matrices.

Inverse Representation: Introduces InvArrowheadMatrix for representing the inverse of an ArrowheadMatrix without explicitly computing it.

Performance Optimizations: Utilizes the special structure of arrowhead matrices to achieve better time and space complexity compared to dense matrix operations.

Implementation Details

ArrowheadMatrix: Stores only the diagonal elements and the "arrow" parts of the matrix.
mul!: Optimized matrix-vector multiplication.
linsolve!: Efficient linear system solver tailored for arrowhead structure.
InvArrowheadMatrix: Lazy representation of the inverse, computing elements only when needed.

@wouterwln
Copy link
Member

Can you recheck the PR? For example, this adds BenchmarkTools to the dependencies of BayesBase

@Nimrais
Copy link
Contributor Author

Nimrais commented Oct 8, 2024

@bvdmitri Yeah the structure is implemented but there is a nuance. Performance tests do not pass if you can help me or suggest me smt there it would be nice.

@Nimrais
Copy link
Contributor Author

Nimrais commented Oct 8, 2024

Right now it passed but I think it's a fluke...

The only tests that fails are promote_samplefloattype but they are also failing in the main @wouterwln.

@wouterwln
Copy link
Member

Yes those tests fail because of the eltype stuff that is in Distributions.jl that I referenced last week

@Nimrais
Copy link
Contributor Author

Nimrais commented Oct 8, 2024

@wouterwln

Can you recheck the PR? For example, this adds BenchmarkTools to the dependencies of BayesBase

This PR is not ready for merging. It's many things missed here (docs for example) but I want to solve the main issue first: ArrowheadMatrix should be faster for inv(A)*b operation. And I need help with that.

@wouterwln wouterwln marked this pull request as draft October 8, 2024 12:43
@Nimrais Nimrais marked this pull request as ready for review October 10, 2024 07:45
@wouterwln
Copy link
Member

@Nimrais can you double check coverage? I cannot see branch coverage and PR coverage. Maybe this is a problem with the bayesbase repo?

Copy link
Member

@bvdmitri bvdmitri left a comment

Choose a reason for hiding this comment

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

Super nice work @Nimrais !!

@bvdmitri bvdmitri merged commit 535238a into main Oct 11, 2024
3 checks passed
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.

3 participants