Add fast path for _colorediteration! with sparse J #124
Conversation
Partial fix for JuliaDiff/SparseDiffTools.jl#138 JuliaDiff/SparseDiffTools.jl#100 Adds a fast path for the case where J and sparsity are are both SparseMatrixCSC and have the same number of columns and stored values.
src/iteration_utils.jl
Outdated
| common_sparsity = (length(J.colptr) == length(sparsity.colptr) && | ||
| length(J.nzval) == length(sparsity.nzval)) |
There was a problem hiding this comment.
I'm not sure about this. This just means that they have the same data size. Now, this might be fine because I can't think of a reason for someone hitting this case on purpose: the reason for sparsity vs J is so that you can say keep J dense but still use color differentiation (for smaller matrices), or do partial/approximate coloring. But if the sparsity has the same sparsity as the matrix you're trying to fill, then partial/approximate coloring doesn't make sense... so I think this is fine. I'll merge it under this understanding but it's good to have it mentioned.
There was a problem hiding this comment.
Can there be a test on this behavior?
There was a problem hiding this comment.
Updated version simplifies this logic so that the fast path is only taken if J and sparsity have the same sparsity pattern (by comparing colptr and rowval).
There is an existing test (in coloring_tests.jl) for color differentiation writing to a dense J
This does reveal what I think might be unrelated pre-exisiting issues with accumulating components of a Jacobian into J, but I'm not sure what the use cases are here:
- in general,
Jis initialised to zero, and elements are set, not added (so this isn't useful to accumulate) - if
Jis sparse, but with different stored elements tosparsityit's not clear to me what the correct behaviour should be:- error ? (instead of silently taking a slow path)
- reset
Jto the pattern ofsparsity? (might make sense if the intent is to setJ. Would allocate, but could be optimized) - keep
Jpattern, initialize to zero, and add new stored elements if needed ? (current behaviour)
There was a problem hiding this comment.
Yeah, the purpose I was thinking of was that, maybe you don't have just dense vs sparse at different parts of the code, but also do something like partial coloring so the sparsity may be in general Tridiagonal while the sparse matrix is not tridiagonal, i.e. using coloring on a subset so that you can use fewer f calculations and get an approximation to the full Jacobian. But if you do that, wouldn't you then want to just specialize the linear system solve on the approximate tridiagonal-ness? So I think this flexibility is somewhat left open as a research question.
But if it's two sparse matrices, I would assume it would only make sense if one has a subset of zeros of the other.
Simplifies fast path logic. All other cases (J is not a SparseMatrixCSC, or J has a different sparsity pattern) use previous slow path.
Partial fix for JuliaDiff/SparseDiffTools.jl#138
JuliaDiff/SparseDiffTools.jl#100
Adds a fast path for the case where J and sparsity are are both
SparseMatrixCSC and have the same sparsity pattern