-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
Support user-specified W_prototype <: AbstractSciMLOperator
#1996
Conversation
67ea0a4
to
36f4048
Compare
equivalent to before)
@ChrisRackauckas, @utkarsh530, @vpuri3 this is finally ready:) |
why do we need a type? why is it not just integrator.gamma = ScalarOp(0.0; update_func)
integrator.W = integrator.M - integrator.gamma * integrator.J |
You mean why not remove the |
LGTM. Yes, I believe |
@gaurav-arya can we resolve this here? |
test/interface/wprototype_tests.jl
Outdated
# TODO: in future, ensure and test that polyalg chooses the best linear solve for all. | ||
sol = solve(prob, Rosenbrock23(linsolve = KrylovJL_GMRES())) | ||
sol_J = solve(prob_J, Rosenbrock23(linsolve = KrylovJL_GMRES())) # note: direct linsolve in this case is broken, see #1998 | ||
sol_W = solve(prob_W, Rosenbrock23(linsolve = KrylovJL_GMRES())) |
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.
Test a newton solver as well, like FBDF. Also test Radau5
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.
Added an FBDF test. RadauIIA5 does not seem to support sparse Jacobians though:
OrdinaryDiffEq.jl/src/caches/firk_caches.jl
Line 238 in b807375
W2 = similar(J, Complex{eltype(W1)}) |
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.
Yeah Radau5 will need more work.
WOperator
to match that of a SciMLOperator.W_prototype <: AbstractSciMLOperator
.Dependent on SciML/SciMLBase.jl#473 and SciML/StochasticDiffEq.jl#540. The intent is for this feature to be undocumented until it is more battle tested. Needs tests
(I have a much more involved set of changes that replaces the existing
WOperator
with a lazy SciMLOperator. But I've come to the conclusion that this is not something to be done lightly, e.g. there aremuladd
optimizations that are not easy to replace with the lazy purely general approach. Instead, I'm just addingAbstractSciMLOperator
branches to first allow the W-operator to be manually swapped out for anAbstractSciMLOperator
, while keeping the originalWOperator
around.)