-
-
Notifications
You must be signed in to change notification settings - Fork 206
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
Sparse Jacobians #416
Comments
I took a look at the code around the jacobian and W operator and I think it's best if we work out the big picture first before actual coding. Currently, the related pieces are:
To summarize: information for the jacobian/W is stored in two places, the Now onto the central issue: the flow of information is missing from
I can see advantages to both solutions. 1 requires fewer changes to the existing codebase, while 2 is in my opinion more logical because these should be considered a part of (By the way one convenience of 2 is that if a Some other issues that have come to my mind:
|
This isn't thread-safe which will give issues with things like DiffEqMonteCarlo. We should keep thread-safety especially with the coming threading changes. I wanted to treat that first because I want it to be clear that is a good we want to keep in mind.
I wouldn't use "fewer changes" as a metric. Our code base slapped this kind of support on as it needed and the whole Jacobian part is in need of a re-design. @YingboMa helped out a lot by refactoring it so now only a small amount of code needs to be targeted (before there was a bunch of copy paste programming... it grew fast...), and now we should use this structure to directly update it correctly.
That's a very good argument for 2. Let's go with 2. Well... another thing we should ask is whether the DiffEqFunction as separate is a good idea. After going through with all of it, could it be better to slap all of those fields into the problem type itself? I don't want to dwell too much on details since it's always easy to move this stuff around and don't want to delay actual work due to bikeshedding. One thing to keep in mind is the lean-ness of the of the inputs. I opened up JuliaLang/julia#25107 to try and find out a leaner way to give matrices than just giving the entire matrix since that's super memory inefficient if we are going to be copying. I don't think we have a better solution for now but this is why this stuff was kind of kept separate in case the functions are copied more. We ended up copying functions less in DelayDiffEq.jl than the original design so it may not be an issue anymore.
Your composition solution gets rid of this whole issue, or at least changes the discussion. The question before was, how do we know when to build
Good idea.
In-place already supports ForwardDiff.jl and DiffEqDiffTools.jl. That's what this is all for: https://github.com/JuliaDiffEq/OrdinaryDiffEq.jl/blob/master/src/derivative_wrappers.jl . |
That's a good point. Yes I will tackle this first in DiffEqOperators, probably via
Oh I see. I was under the impression that there might be cases where W is preferred to be calculated as just a dense matrix. Since lazy composition is preferred there is indeed no need for |
Well it's only ever used to linear solve, I think it's fine to hand the composed operator over to the users and let them make the choice for how to handle it, with the default routines doing the same thing as before which is just materializing the matrix and then factorizing. To get to where we were before, we'd want to have an allocation free way of materializing that though, but that sounds like a generally useful thing to plan for the operator handling anyways. |
I can see where this is going. Now that every I looked into
About this... I realized DiffEqOperators is not a requirement of DiffEqBase so this is not possible. Of course I can just add it to REQUIRE but I'm not sure if this is a good idea from a package development standpoint. Instead, I can add an additional clause to |
No, it will be thread-safe since deepcopy will make a copy of those internal variables.
While having all of the extra operators defined in a separate library is nice, I'm not sure all of the structure should've been moved to a separate package. |
Ah wait, I can finally see why it is a good idea to define the abstract interface for the operators in DiffEqBase and not have DiffEqOperators be self-contained ;) |
Yeah, I also like option 2.
I would like to make the function |
Now that work on lazy W is starting, I'd like to discuss the variable Conceptually, the lazy W operator can be constructed as (for the moment let's make it simple and assume both γ = DiffEqScalar(dtgamma)
W = mass_matrix - γ * J If
|
I don't like 1. Constructing the lazy W each time is cheap so I think that's fine, but making it its own special type could be useful since it will be user-facing in the linsolve interface. What kind of interface can we put on it and would it make it much easier for users to interact with than having it be a standard operator? I think it would be cool if if !have_cache(W)
allocate_cache!(W)
end
if matrix_updated
materialize!(W) # Writes to the internal cache the actual W matrix
p.Wfac = factorize!(W) # uses the internal cache
end
ldiv!(x,p.Wfac,b) essentially this is saying " gmres(x,W,b) and use it directly without factorizing. Thoughts? |
Interesting. Kinda reminds me of lazy constructs in languages such as C#. But I think option 3 is the way to go by the looks of it. Having a dedicated type for W makes it way more versatile than generic composition, your allocate-as-needed cache being one example. I will start with purely lazy |
SciML/DifferentialEquations.jl#220 defines an interface an the whole of DiffEq. Implementing it into OrdinaryDiffEq.jl well will be an ordeal as well. We will need to change the W calculations to take into account possible structure, sparsity, or laziness:
https://github.com/JuliaDiffEq/OrdinaryDiffEq.jl/blob/master/src/derivative_utils.jl#L35
Specifically these lines:
https://github.com/JuliaDiffEq/OrdinaryDiffEq.jl/blob/master/src/derivative_utils.jl#L66-L73
We will probably want to handle SciML/DifferentialEquations.jl#259 and SciML/DifferentialEquations.jl#263 simultaneously by updating this one https://github.com/JuliaDiffEq/OrdinaryDiffEq.jl/blob/master/src/derivative_utils.jl#L82 to have similar options to the in-place one.
Then we just need to make https://github.com/JuliaDiffEq/OrdinaryDiffEq.jl/blob/master/src/caches/kencarp_kvaerno_caches.jl#L73-L74 build the correct cache depending on the
jac_prototype
. That should probably be some dispatching function inderivative_utils.jl
that handles it that we then apply to every cache similarly. In the caches, we will want to make the default linsolve change depending on the type of J/W here: https://github.com/JuliaDiffEq/OrdinaryDiffEq.jl/blob/master/src/caches/kencarp_kvaerno_caches.jl#L92 . So it should probably benothing
in the algorithms and thenif nothing
use a smarter defaultelse
use the user-override. For sparse it's probably smart to just use the sparse-LU, and for matrix-free we can just default to GMRES (maybe add in a simple preconditioner?) from IterativeSolvers.jl.Then we will need to incorporate it into https://github.com/JuliaDiffEq/OrdinaryDiffEq.jl/blob/master/src/nlsolve_utils.jl . We should have a dispatch to avoid the whole nonlinear solving if the function is linear. While
u' = Au
isn't much of an interesting case, the IMEX algorithms applied to semilinear equationsu' = Au + f(t,u)
is one case where this will show up so we might as well handle it. I don't think any more modifications will be needed there.Lastly, the exponential integrators need to make use of it. I think they are setup just to use the matmul, so the previous parts should already be enough to get them using the correct sparse/matrix-free Jacobians.
@YingboMa 's and @MSeeker1340 's work will make heavy use of this, so since this is a large change we may want to coordinate doing this in pieces.
The text was updated successfully, but these errors were encountered: