Open isaacsas opened 2 months ago
My plan is to create a keyword argument alias
which then has a struct ODEAliases <: AbstractAliasSpecifier
etc., that holds booleans/nothing for each thing that can be aliased. nothing
for default behavior, true/false
for forced aliasing, de-aliasing. This can then be rolled out for every problem type uniformly, and include booleans to cover the keyword arguments.
@jClugstor does this specification make sense?
You'll find:
alias_u0: allows the solver to alias the initial condition array that is contained in the problem struct. Defaults to false.
in DiffEq
alias_A::Bool: Whether to alias the matrix A or use a copy by default. When true, algorithms like LU-factorization can be faster by reusing the memory via lu!, but care must be taken as the original input will be modified. Default is true if the algorithm is known not to modify A, otherwise is false. alias_b::Bool: Whether to alias the matrix b or use a copy by default. When true, algorithms can write and change b upon usage. Care must be taken as the original input will be modified. Default is true if the algorithm is known not to modify b, otherwise false.
in LinearSolve. Etc. All of that should get folded into just these more general structs, and the old way should get deprecated to just setting values of the struct.
I think I get it. alias
should be a keyword of ODEProblem
, LinearProblem
, etc. that would be of type e.g. ODEAliases
and LinearAliases
respectively. And each concrete subtype of AbstractAliasSpecifier
should hold boolean values that correspond to the parts of their corresponding problems that can be aliased.
Yes exactly.
Should the types ODEAliases
and LinearAliases
etc. be defined in SciMLBase?
yes
I'm thinking that the problem types will need a new field to hold the AbstractAliasSpecifier
s, does that sound right?
I don't think so, they will just be a new common kwarg to solve.
These four PRs should add everything that's needed to use this for ODEs and Linear Problems.
Basically just like you said, I added an AbstractAliasSpecifier
in SciMLBase, and then subtypes can be defined for every problem type. In DiffEqBase all I needed to do was add alias
to the acceptable kwargs list.
Oh and I'm not sure I got all of the version bumps right, but I think it's just that LinearSolve and OrdinaryDiffEqCore need to be using SciMLBase@2.58
and DiffEqBase@6.160
for it all to work.
https://github.com/SciML/DiffEqBase.jl/pull/1099 https://github.com/SciML/SciMLBase.jl/pull/830 https://github.com/SciML/LinearSolve.jl/pull/553 https://github.com/SciML/OrdinaryDiffEq.jl/pull/2503
The interface needs a few more things. We should define the interface in AbstractAliasSpecifier
to have:
alias = nothing
, which if true
would override all other options and set all aliasing to true. If false
, it would override all to false
. By default it should just be nothing
.alias
in solve to also take true/false
which pushes it down to alias=true/false
in the AbstractAliasSpecifier
.alias_*
keyword arguments for the general alias
one. As a non-breaking change, the concrete AbstractAliasSpecifier
s should match the keyword arguments that exist by default.alias_p
and alias_f
## Keyword Arguments
and a list instead of just inline text.Any other things you see @isaacsas @ranocha @avik-pal ?
There is also a kwarg alias_du0
, e.g, https://github.com/SciML/OrdinaryDiffEq.jl/pull/2503/files#diff-28a5b9e0abdb761fcac81be080bd7bb47c49f41c9d6b18df041d51881f6b1f41R73
- Deprecate the existing
alias_*
keyword arguments for the generalalias
one. As a non-breaking change, the concreteAbstractAliasSpecifier
s should match the keyword arguments that exist by default.
I think we have to take this one step further - if users just use the old API, everything has to work as documented. See, e.g., https://github.com/SciML/OrdinaryDiffEq.jl/pull/2503/files#r1818084222
Having an alias option around the tstop
s vector would also be nice, and was the original motivation for this issue. I was just going to add it to JumpProcesses, but held off to use the aliasing system.
I think we have to take this one step further - if users just use the old API, everything has to work as documented. See, e.g., https://github.com/SciML/OrdinaryDiffEq.jl/pull/2503/files#r1818084222
Yes, that's what I meant by the deprecation path. It shouldn't change, just be extended with a deprecation warning.
There is also a kwarg alias_du0
Having an alias option around the tstops vector would also be nice, and was the original motivation for this issue. I was just going to add it to JumpProcesses, but held off to use the aliasing system.
Yes, and these two are examples of kwargs to specific instances of AbstractAliasSpecifier
, specifically ODEAliasSpecifier
should document and add those two.
So the full list for ODEAliases
for example would be
struct ODEAliases <: AbstractAliasSpecifier
alias::Union{Bool,Nothing}
alias_p::Union{Bool,Nothing}
alias_f::Union{Bool,Nothing}
alias_u0::Union{Bool,Nothing}
alias_du0::Union{Bool,Nothing}
alias_tstops::Union{Bool,Nothing}
end
``` ?
struct ODEAliases <: AbstractAliasSpecifier
alias_p::Union{Bool,Nothing}
alias_f::Union{Bool,Nothing}
alias_u0::Union{Bool,Nothing}
alias_du0::Union{Bool,Nothing}
alias_tstops::Union{Bool,Nothing}
end
alias
is just a high level kwarg for simplicity.
A couple of questions:
Are there any solvers where alias_p
and alias_f
should actually do anything at this point? I haven't found any solvers where these are keywords. I assume alias_p
means alias the p
parameter array, what should alias_f
do?
So far the only other variable I can find that explicitly mentions any aliasing is alias_u0
in NonlinearSolve
. Are there any other solvers that already have an aliasing option that I need to change to make use of the aliasing API?
Are there any solvers where alias_p and alias_f should actually do anything at this point? I haven't found any solvers where these are keywords.
No, those are ones to add.
I assume alias_p means alias the p parameter array, what should alias_f do?
Yes, and f is for whether it should double prob.f
, i.e. integrator.f = deepcopy(prob.f)
, to decouple caches from the user's function.
Got it. I should be able to implement these by changing things in __init
right? I shouldn't have to change anything in the actual solver code?
Yes exactly. You'll find that there's places in the __init
definitions where things just do p = prob.p
, and that builds the aliasing. It would need to have options to copy instead.
Per discussions @ChrisRackauckas and I had around https://github.com/SciML/JumpProcesses.jl/issues/442 it would be nice to be able to have users tell us we can alias their
tstops
vector in JumpProcesses so we don't need to allocate a new vector for each call tosolve
.