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

Make cholmod vector solve return vector #320

Merged
merged 3 commits into from
Jan 18, 2023
Merged

Make cholmod vector solve return vector #320

merged 3 commits into from
Jan 18, 2023

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch requested a review from rayegun January 4, 2023 16:12
@@ -1526,7 +1544,10 @@ end
function (\)(L::FactorComponent, B::Matrix)
Matrix(L\Dense(B))
end
function (\)(L::FactorComponent, B::SparseVecOrMat)
function (\)(L::FactorComponent, B::SparseVector)
sparsevec(L\Sparse(B))
Copy link
Contributor

@stevengj stevengj Jan 4, 2023

Choose a reason for hiding this comment

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

I'm a little confused about why we are returning a sparse vector — isn't the result generally dense?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was confused about that myself, but note that previously we returned a SparseMatrixCSC. This change only changes the dimension of the returned array. But, of course, we may as well change to dense return arrays. The actual type of the return value doesn't seem to be tested, so I don't know what "promises" we made.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we might as well stay consistent here.

See also https://discourse.julialang.org/t/sparse-solve-with-sparse-rhs/81595/3?u=stevengj

@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2023

Codecov Report

Merging #320 (cd757e4) into main (31b491e) will decrease coverage by 0.00%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##             main     JuliaLang/julia#320      +/-   ##
==========================================
- Coverage   93.71%   93.71%   -0.01%     
==========================================
  Files          12       12              
  Lines        7433     7446      +13     
==========================================
+ Hits         6966     6978      +12     
- Misses        467      468       +1     
Impacted Files Coverage Δ
src/solvers/cholmod.jl 90.99% <94.73%> (+0.02%) ⬆️
src/linalg.jl 95.56% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@stevengj
Copy link
Contributor

stevengj commented Jan 4, 2023

LGTM.

@ViralBShah
Copy link
Member

Merge?

@dkarrasch dkarrasch merged commit c7ad0b9 into main Jan 18, 2023
@dkarrasch dkarrasch deleted the dk/vecsolve branch January 18, 2023 19:38
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.

Solving sparse linear system returns a Matrix
4 participants