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

Changes required for initialization #470

Open
AayushSabharwal opened this issue Jan 21, 2025 · 6 comments
Open

Changes required for initialization #470

AayushSabharwal opened this issue Jan 21, 2025 · 6 comments

Comments

@AayushSabharwal
Copy link
Member

JumpProblem has a unique set of problems to solve for initialization. Ignoring SSAStepper for a moment, JumpProblem wraps another problem and relies on the integrator of the wrapped problem. It wraps the inner prob.u0 in an ExtendedJumpArray containing extra values. This makes initialization problematic since initializeprobmap returns a Vector{Float64} which does not have the required extra values, thus leading to an error.

ExtendedJumpArray is not public API, and hence MTK cannot handle this as an edge case. One solution discussed is to wrap the inner prob.u0 in an ExtendedJumpArray when the user calls solve/init. However, this is still problematic:

  • We'd need to allocate the array for the required extra values every solve call
  • Each solve call would call init on the wrapped integrator twice: once just to run initialization, and a second time with NoInit to change the type of integrator.u to the appropriate ExtendedJumpArray.

Another potential solution is for JumpProcesses to take ownership of initialization - solve for the updated values, update the ExtendedJumpArray, and call init on the inner problem with NoInit. The downside here is mostly just implementation. We can avoid a NonlinearSolve dependency by making it a weakdep, in a similar approach to how OrdinaryDiffEqNonlinearSolve extends OrdinaryDiffEqCore.default_nlsolve.

@isaacsas
Copy link
Member

We discussed on Slack adding a cache array into JumpProblem to avoid allocating when wrapping. The one drawback is this moves us away from JumpProblem being immutable, which I had thought was a longer-term goal to be more consistent with other problem types.

Otherwise we'd need to move from an interface where people call solve in a loop to generate samples to telling them to use the integrator interface and reinit (along with adding/checking reinit support here for SSAStepper and such). That might be the better approach, but it does put more onto users.

@ChrisRackauckas
Copy link
Member

This might just be the right time to rewrite this. We don't need the ExtendedJumpArray. We can just do it with integration and have it trigger reversals. This is similar to improved adjoint techniques. I'll need to find some time...

@isaacsas
Copy link
Member

If we are going to do that, it might also be the time to think about moving the aggregator initialization into init instead of JumpProblem (though again, for efficiency reasons one then needs to use reinit when running many solves). i.e. put all the big breaking changes together at once.

@isaacsas
Copy link
Member

(that would, I think, let us make JumpProblem immutable.)

@AayushSabharwal
Copy link
Member Author

If this is going to take a while, I'd rather opt out of initialization for JumpProblem in MTK and get SciML/ModelingToolkit.jl#3347 merged

@ChrisRackauckas
Copy link
Member

That's probably the right way to do this for now.

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

3 participants