make lu! allocate less if possible #104
Conversation
Codecov Report
@@ Coverage Diff @@
## main #104 +/- ##
==========================================
- Coverage 34.25% 32.92% -1.33%
==========================================
Files 25 25
Lines 21127 21121 -6
==========================================
- Hits 7237 6955 -282
- Misses 13890 14166 +276
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
can we change the to in SparseArrays.jl/src/solvers/umfpack.jl Line 497 in 23bc7c9 ? Also lu! isn't safe in the sense that calling an lu! on a matrix with a different sparsity pattern will result in an error since we reuse the symbolic SparseArrays.jl/src/solvers/umfpack.jl Line 510 in 23bc7c9 but we also support changing the size of the matrix. One solution would be to provide an argument to recalculate the symbolic. Another would be to just not let any one change the size of the the lu. @Wimmerer what do you think? |
|
We should probably check that the input pattern is the same when users pass in an entirely new |
|
Also what do you mean reuse the numeric? |
|
i said reuse the symbolic not numeric. Every time umfpack calculates the lu it first calculates the symbolic and then the numeric. The symbolic can be reused if the sparsity pattern is the same (not sure what the speed up is). |
Is what you wrote above, hence my confusion. Reusing is fine for symbolic, and I'm surprised it wasn't done already. I do it in KLU.jl. We need to expose the condition number estimates and such if we don't already. You should check those when refactoring. |
|
Ah, you were referring to the comment above, yeah i don't think we can reuse the numeric. To be clear sparse arrays does reuse it. should i leave the error handling to umfpack? or should we also do the checks on the julia side? what's a condition number estimates? |
|
Leave what error handling you can to UMFPACK, we should be wrapping exceptions there correctly.
|
Yes, should be ok to do so. Do you want to add that in this PR? |
|
yes, i'm a bit swamped at the moment, hopefully will have some time to do these changes in a few days. |
|
No urgency at all. I just have been going through all the issues and PRs today. Needless to say, thanks for all your contributions. |
As a general rule of thumb, I believe symbolic is about 10-15% of the total computation time. |
|
good to know |
|
@Wimmerer @ViralBShah i think this is good to go |
|
We shouldn't need locks anymore now that Control, Info, and workspaces are local to each Factorization right? The correct usage pattern is not to use the locks, but to duplicate the control, info, and workspaces. |
|
i guess, should i remove them? i think that they don't really cost anything and save the headache from unsuspecting people as it's really not intuitive that ldiv! changes the factorization. |
|
also should i add a small footnote about parallel solving to the documentations? like it scales well and stuff i'd use something like @floop for i in axes(b, 2)
@init w = duplicate(F)
ldiv!(view(x, :, i), w, view(b, :, i))
end |
|
Well I wouldn't put Floops in the official docs yet. But Threads should be good yes. |
|
@Wimmerer i'm still not sure, do i keep the locks? i'll put the parallel stuff in another commit. |
|
Someone more experienced with the pros and cons of locks should probably chime in here :/ Relative to a solve call they're probably a miniscule time loss, so I lean towards leaving them in now. |
|
@Wimmerer Is this good to merge? |
|
Do the locks make sense to you? It prevents mistakes I guess, even if the intended usage is a thread local view of each Factor. |
|
Yes, the locks are fairly coarse grained and look ok. @SobhanMP Can we have some tests that do multi-threaded LUs? |
|
I believe they mentioned including those in a separate PR. I may be thinking of docs though. We can merge this part whenever ready. |
|
Yes, I see multi-threaded LU tests in the testsuite - but wasn't sure if more were needed as part of this PR. |
|
Actually, let's hold on this one and get an stdlib bump first to get the other recent PRs into Julia nightly - and then merge this one. |
|
What's the reason to close? (out of curiosity). |
|
github bugged, i wanted to make a new branch called main, i thought rename the branch would work |
|
gonna open a new one, if you don't mind |
While at this point it should make things a bit nicer, i have two questions:
1- is there any downside to using broadcase, should i write the loop? and,
2- can we reuse the numeric in umfpack?
Right now, it resizes the fields of the umfpacklu instead of reallocating.