feat: add fields to OverrideInit, better nlsolve_alg handling
#857
Conversation
AayushSabharwal
force-pushed
the
as/get-nlsolve
branch
from
924e92b to
66f5229
Compare
AayushSabharwal
changed the title
override_init_get_nlsolveOverrideInit, better nlsolve_alg handling
AayushSabharwal
force-pushed
the
as/get-nlsolve
branch
from
66f5229 to
9ab1a71
Compare
src/initialization.jl
Outdated
| throw(OverrideInitMissingAlgorithm()) | ||
| end | ||
|
|
||
| if initdata.update_initializeprob! !== nothing | ||
| initdata.update_initializeprob!(initprob, valp) | ||
| end | ||
|
|
||
| nlsol = solve(initprob, nlsolve_alg) | ||
| nlsol = solve(initprob, nlsolve_alg; abstol = alg.abstol) |
There was a problem hiding this comment.
it should default to using the tolerance from the solver if not supplied elsewhere.
| @@ -100,7 +100,8 @@ Check if the algebraic constraints are satisfied, and error if they aren't. Retu | |||
| the `u0` and `p` as-is, and is always successful if it returns. Valid only for | |||
| `ODEProblem` and `DAEProblem`. Requires a `DEIntegrator` as its second argument. | |||
| """ | |||
| function get_initial_values(prob::ODEProblem, integrator, f, alg::CheckInit, | |||
| function get_initial_values( | |||
| prob::AbstractDEProblem, integrator::DEIntegrator, f, alg::CheckInit, | |||
There was a problem hiding this comment.
This doesn't handle DAEProblem?
There was a problem hiding this comment.
There's an AbstractDAEProblem dispatch below
There was a problem hiding this comment.
But then this one should just be for ODEProblem?
There was a problem hiding this comment.
SDEProblem dispatches here too
| @@ -109,7 +110,7 @@ function get_initial_values(prob::ODEProblem, integrator, f, alg::CheckInit, | |||
|
|
|||
| algebraic_vars = [all(iszero, x) for x in eachcol(M)] | |||
| algebraic_eqs = [all(iszero, x) for x in eachrow(M)] | |||
| (iszero(algebraic_vars) || iszero(algebraic_eqs)) && return | |||
| (iszero(algebraic_vars) || iszero(algebraic_eqs)) && return u0, p, true | |||
| update_coefficients!(M, u0, p, t) | |||
| tmp = _evaluate_f_ode(integrator, f, isinplace, u0, p, t) | |||
| tmp .= ArrayInterface.restructure(tmp, algebraic_eqs .* _vec(tmp)) | |||
There was a problem hiding this comment.
integrator.opts.internalnorm: This doesn't handle Sundials or ODEInterface?
There was a problem hiding this comment.
Hmm... I didn't know that. Makes sense that SciMLBase can't make the same assumptions OrdinaryDiffEq did
AayushSabharwal
force-pushed
the
as/get-nlsolve
branch
from
b5a156a to
3cf6e49
Compare
src/alg_traits.jl
Outdated
| """ | ||
| $(TYPEDSIGNATURES) | ||
|
|
||
| Get the `abstol` for the solve stored in the cache. Required for caches of algorithms for | ||
| which `cache_stores_tolerances(alg) == true`. | ||
| """ | ||
| function cache_get_abstol(cache) | ||
| error("Cache does not store the `abstol`.") | ||
| end | ||
|
|
||
| """ | ||
| $(TYPEDSIGNATURES) | ||
|
|
||
| Get the `reltol` for the solve stored in the cache. Required for caches of algorithms for | ||
| which `cache_stores_tolerances(alg) == true`. | ||
| """ | ||
| function cache_get_reltol(cache) | ||
| error("Cache does not store the `reltol`.") |
There was a problem hiding this comment.
I thought we talked about passing it?
There was a problem hiding this comment.
Didn't you also mention having a trait and throwing an error message? Right now it takes a keyword argument and falls back to this if not provided.
There was a problem hiding this comment.
That said I'm more than happy to remove this trait since solvers have to individually opt-in to doing initialization anyway.
There was a problem hiding this comment.
Let's simplify and just make it required.
AayushSabharwal
force-pushed
the
as/get-nlsolve
branch
2 times, most recently
from
0f41da2 to
436f8ab
Compare
|
A lot of our tests still use codecov, apparently 😅 |
AayushSabharwal
force-pushed
the
as/get-nlsolve
branch
from
436f8ab to
7eb3546
Compare
AayushSabharwal
force-pushed
the
as/get-nlsolve
branch
from
7eb3546 to
5e572e3
Compare
AayushSabharwal
force-pushed
the
as/get-nlsolve
branch
from
5e572e3 to
7f93fb2
Compare
|
This is good to go. Downstream failures are BVDiffEq failing to precompile. |
This replaces
default_nlsolvein OrdinaryDiffEq, and will have methods in NonlinearSolve which can then be leveraged by all libraries for initialization.Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.