-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Mass matrix change #126
Mass matrix change #126
Conversation
@@ -83,10 +77,10 @@ function SecondOrderODEProblem(f::DynamicalODEFunction,du0,u0,tspan,p=nothing;kw | |||
v | |||
end | |||
end | |||
return ODEProblem(DynamicalODEFunction{iip}(f.f1,f2;analytic=f.analytic),_u0,tspan,p, | |||
return ODEProblem(DynamicalODEFunction{iip}(f.f1,f2;mass_matrix=f.mass_matrix,analytic=f.analytic),_u0,tspan,p, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a tuple mass matrix (mm,I)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or formalize it as two mass matrix fields: mm1
and mm2
in DynamicalODEFunction. This might be more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DynamicalODEFunction
should always have a tuple (mm1, mm2)
as its mass matrix? (I can probably make this a requirement in the type declaration).
If I understand it correctly, the functions here are intended to convert a DynamicalODEProblem
to a SecondOrderODEProblem
right? If this is the case, then I believe we should inherit the mass matrix from the original problem/function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Dynamical ODE is a 2-partitioned ODE, so it could have two mass matrices:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Dynamical ODE is a 2-partitioned ODE, so it could have two mass matrices
I'm slightly confused. If what you're referring to is separate mass matrix for the u
and v
parts, then the current implementation already uses a 2-tuple mass matrix and I did the same with DynamicalODEFunction
.
I think keep a single mass_matrix
field makes the interface clearer. After all, having f.mm1
and f.mm2
is functionally the same as having f.mass_matrix = (mm1, mm2)
. Of course, if we use a getter method instead, then none of these are needed as we can just get_mm(f::DynamicalODEFunction) = (get_mm(f.f1), get_mm(f.f2))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mass_matrix=f.mass_matrix
that's passing on a tuple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh misread it, my bad.
src/diffeqfunction.jl
Outdated
@@ -260,22 +281,27 @@ function SDEFunction{iip,false}(f,g; | |||
paramjac = nothing, | |||
ggprime = nothing, | |||
syms = nothing) where iip | |||
# Is this still necessary? | |||
if mass_matrix == I && typeof(f) <: Tuple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah let's take this out.
src/diffeqfunction.jl
Outdated
@@ -169,46 +180,50 @@ function ODEFunction{iip,false}(f; | |||
invW_t=nothing, | |||
paramjac = nothing, | |||
syms = nothing) where iip | |||
# Is this still necessary? | |||
if mass_matrix == I && typeof(f) <: Tuple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's take this out.
One thing I forgot to mention is how the default mass matrix for Of course there's also the alternative option of dropping get_mm(f::ODEFunction) = f.mass_matrix
get_mm(f::SplitFunction) = get_mm(f.f2)`
get_mm(f::DynamicalODEFunction) = (get_mm(f.f1), get_mm(f.f2)) This is a bit restrictive since we cannot override the mass matrix for |
|
Seeing that I think that, just like seeing a regular |
I'm just not really sure what that generalization would even mean physically. |
For split functions, it's IMEX so |
LGTM. I don't see a |
These are the proposed changes to how mass matrix should be handled in DiffEq. Basically, the
mass_matrix
field is no longer stored as a field of*DEProblem
but instead*DEFunction
.The main reason for these changes is to make sparse/lazy W operator handling in OrdinaryDiffEq simpler SciML/OrdinaryDiffEq.jl#416 (comment). Apart from this, there are also be cases where knowing what the mass matrix is solely by querying the
*DEFunction
might help. One case I can think of is converting a problem with non-unity mass matrix to a regular problem, i.e.Mu' = f(u)
->u' = M^(-1)f(u) = g(u)
. Since a lot of integrators do not use the mass matrix explicitly (e.g. the ExpRK integrators) this can come handy. By storingmass_matrix
inODEFunction
we can do something like:An additional benefit is when a single
*DEProblem
type support multiple*DEFucntion
. For example,DynamicalProblem
,SecondOrderODEProblem
andSplitODEProblem
are allODEProblem
in disguise, each with their own*DEFunction
type. Whereas previouslyODEProblem.mass_matrix
need to handle all these cases, now the mass matrices are handled by the functions and can thus allow specialization (e.g. incalc_W!
).These changes are only the first step though. @ChrisRackauckas can you create a new branch for DiffEqBase with these changes, so that I can update OrdinaryDiffEq and other packages using it as a base?