-
-
Couldn't load subscription status.
- Fork 235
Adaptive IDSolve #2881
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
base: master
Are you sure you want to change the base?
Adaptive IDSolve #2881
Conversation
…ds generic adaptivity
| function empty(u_next, u, p, t) | ||
| nothing | ||
| end | ||
| # @testset "Handle nothing in u0" begin |
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.
I have trouble understanding the purpose of this test. What exactly is the practical scenario here?
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.
we use u=nothing to represent length 0 u in a type stable manner (as op0osed to a Vector which is length 0 at runtime)
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.
Sorry, I think I do not understand the point - do you imply here that a zero-length solution vector not type stable and this is why we resort to "nothing"?
When I try to get this test running, I get errors from an alias analysis function not being dispatched for nothing in NonlinearSolve (https://github.com/SciML/NonlinearSolve.jl/blob/ac9344f9359833282e443c4479427ad9ce3311dd/lib/NonlinearSolveFirstOrder/src/solve.jl#L157).
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.
Inplace functions with empty return types are also not correctly handled.
using NonlinearSolveFirstOrder
function f(u, p)
nothing
end
prob = NonlinearProblem{false}(f, Float64[], nothing)
iter = init(prob, NewtonRaphson())errors when the termination cache is built, because the increment type cannot be derived.
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.
the point is that zero length u tends to cause problems (e.g. solvers will try to look at the first element of the state to find a type), so this way you can skip the solve process since nothing interesting can happen if you don't have any state. See https://github.com/SciML/DiffEqBase.jl/blob/3667bdbdc85489f7b296316df7f4c440519e82f6/src/solve.jl#L31 for how this gets handled for ODEs/DAEs.
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.
Wouldn't it make more sense from a software engineering stand point to replace such isa statements with dispatchable functions to make the code more extensible?
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.
quite possibly. DiffEqBase is not my favorite code organization.
| # @concrete struct ConvergenceRateTracing | ||
| # inner_tracing | ||
| # end | ||
|
|
||
| # @concrete struct ConvergenceRateTraceTrick | ||
| # incrementL2norms | ||
| # residualL2norms | ||
| # trace_wrapper | ||
| # end | ||
|
|
||
| # function NonlinearSolveBase.init_nonlinearsolve_trace( | ||
| # prob, alg::IDSolve, u, fu, J, δu; | ||
| # trace_level::ConvergenceRateTracing, kwargs... # This kind of dispatch does not work. Need to figure out a different way. | ||
| # ) | ||
| # inner_trace = NonlinearSolveBase.init_nonlinearsolve_trace( | ||
| # prob, alg, u, fu, J, δu; | ||
| # trace_level.inner_tracing, kwargs... | ||
| # ) | ||
|
|
||
| # return ConvergenceRateTraceTrick(eltype(δu)[], eltype(fu)[], inner_trace) | ||
| # end |
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.
From my understanding, it should be possible to use NonlinearSolveBase.init_nonlinearsolve_trace to query convergence rate estimates. However, in the current design I cannot add a new dispatch. Should I make a PR to pull the trace level before the kwargs (i.e. NonlinearSolveBase.init_nonlinearsolve_trace(prob, alg, u, fu, J, δu, trace_level; kwargs...) for custom dispatches?
| state = ImplicitDiscreteState(isnothing(u) ? nothing : zero(u), p, t) | ||
| IDSolveCache(u, uprev, state, nothing) | ||
| state = ImplicitDiscreteState(zero(u), p, t) | ||
| f_nl = (resid, u_next, p) -> f(resid, u_next, p.u, p.p, p.t) |
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.
What is the reasoning here to include the current u in the signature of this function, but no information on dt?
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.
dt is just a parameter?
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.
I guess my question is simply, why is
| @@ -0,0 +1,52 @@ | |||
| Base.@kwdef struct KantorovichTypeController <: OrdinaryDiffEqCore.AbstractController | |||
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.
TODO reference and documentation
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 I'm not sure what this is.
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.
Oh, sorry. I will write the docs, no worries. I just left it here so I do not forget before merging. This is a controller derived from a posteriori estimates on how much the convergence radius in the Newton-Kantorovich theorem changes for some increment
| else # :constant | ||
| cache.z .= integrator.u | ||
| end | ||
| state = ImplicitDiscreteState(cache.z, p, t+dt) |
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.
On master we solve at time
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.
the somewhat complicated part here is that we want to match a DiscreteSolve. I forget which is the right way here. @jClugstor might remember.
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.
From what I can tell from the docs https://docs.sciml.ai/DiffEqDocs/stable/types/discrete_types/
if you put in u_n, p, t_(n+1) to the function you get u_(n+1) out, so in this case you would get u_(t + dt) out I guess. So if you want u_(t + dt) I think that's correct. Not entirely sure though.
|
|
||
| isfsal(alg::IDSolve) = false | ||
| alg_order(alg::IDSolve) = 0 | ||
| alg_order(alg::IDSolve) = 1 |
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.
why 1 here?
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.
This comes from the analysis shown in Deuflhard, Newton Methods for Nonlinear Problems (Section 5.1.1, see Equation 5.6 and the surrounding definition).
The algorithms here have an associated order in the sense that for a given
Does that explain it?
*The Davidenko differential equation for
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.
I guess I'm confused. It's a discrete time problem, it's exact?
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.
Okay, let me try to explain it differently then. We have the parametric function
| resize!(Θks, 0) | ||
| residualnormprev = zero(eltype(u)) | ||
| while NonlinearSolveBase.not_terminated(nlcache) | ||
| step!(nlcache) |
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.
should it also be trying jacobian reuse in here?
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.
Yes. However, shoudln't the reuse strategy should be part of the NonlinearSolve algorithm instead of some intermediate layer?
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.
if you expand out the iterator then you're taking over the iteration strategy.
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.
Correct. I would like to remove that later. Right now I cannot use the high level API because I cannot access the convergence rates which I need in the controller.
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context