Skip to content
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

create API for tstop aliasing system #1077

Open
isaacsas opened this issue Aug 31, 2024 · 21 comments
Open

create API for tstop aliasing system #1077

isaacsas opened this issue Aug 31, 2024 · 21 comments
Assignees

Comments

@isaacsas
Copy link
Member

Per discussions @ChrisRackauckas and I had around SciML/JumpProcesses.jl#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 to solve.

@ChrisRackauckas
Copy link
Member

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.

@ChrisRackauckas
Copy link
Member

@jClugstor does this specification make sense?

@ChrisRackauckas
Copy link
Member

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.

@jClugstor
Copy link

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.

@ChrisRackauckas
Copy link
Member

Yes exactly.

@jClugstor
Copy link

Should the types ODEAliases and LinearAliases etc. be defined in SciMLBase?

@ChrisRackauckas
Copy link
Member

yes

@jClugstor
Copy link

I'm thinking that the problem types will need a new field to hold the AbstractAliasSpecifiers, does that sound right?

@ChrisRackauckas
Copy link
Member

I don't think so, they will just be a new common kwarg to solve.

@jClugstor
Copy link

jClugstor commented Oct 25, 2024

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 [email protected] and [email protected] for it all to work.

#1099
SciML/SciMLBase.jl#830
SciML/LinearSolve.jl#553
SciML/OrdinaryDiffEq.jl#2503

@ChrisRackauckas
Copy link
Member

The interface needs a few more things. We should define the interface in AbstractAliasSpecifier to have:

  • A keyword argument 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.
  • Allow for the keyword alias in solve to also take true/false which pushes it down to alias=true/false in the AbstractAliasSpecifier.
  • A contract/guarantee that if all aliases are false, then the solves are thread-safe.
  • Deprecate the existing alias_* keyword arguments for the general alias one. As a non-breaking change, the concrete AbstractAliasSpecifiers should match the keyword arguments that exist by default.
  • Every abstract alias specifier also needs to add alias_p and alias_f
  • The docstrings should follow the standard format of SciMLStyle, so ## Keyword Arguments and a list instead of just inline text.
  • IntegralProblem, OptimizationProblem, NonlinearProblem, and non-ODE DiffEq seem absent from the list right now.

Any other things you see @isaacsas @ranocha @avik-pal ?

@ranocha
Copy link
Member

ranocha commented Oct 27, 2024

@ranocha
Copy link
Member

ranocha commented Oct 27, 2024

  • Deprecate the existing alias_* keyword arguments for the general alias one. As a non-breaking change, the concrete AbstractAliasSpecifiers 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

@isaacsas
Copy link
Member Author

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.

@ChrisRackauckas
Copy link
Member

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.

@jClugstor
Copy link

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
``` ?

@ChrisRackauckas
Copy link
Member

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.

@jClugstor
Copy link

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?

@ChrisRackauckas
Copy link
Member

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.

@jClugstor
Copy link

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?

@ChrisRackauckas
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants