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

Integrator interface for manually checking integrator.sol.retcode? #276

Closed
tkf opened this issue Apr 12, 2018 · 7 comments
Closed

Integrator interface for manually checking integrator.sol.retcode? #276

tkf opened this issue Apr 12, 2018 · 7 comments

Comments

@tkf
Copy link

tkf commented Apr 12, 2018

From grepping solution_new_retcode OrdinaryDiffEq/src, it looks like retcode is set only via @ode_exit_conditions (and so solve!). Is it correct?

If so, I suppose there is no integrator interface for checking the "healthiness" (retcode) of the integrator. It would be nice to have such an interface.

How about something like check_code(integrator) which just returns one of the Return Codes? For OrdinaryDiffEq, it would be straight forward to implement (untested; it was just copy-and-paste + ε of @ode_exit_conditions):

function check_code(integrator)
  if integrator.iter > integrator.opts.maxiters
    if integrator.opts.verbose
      warn("Interrupted. Larger maxiters is needed.")
    end
    return :MaxIters
  end
  if !integrator.opts.force_dtmin && integrator.opts.adaptive && abs(integrator.dt) <= abs(integrator.opts.dtmin)
    if integrator.opts.verbose
      warn("dt <= dtmin. Aborting. If you would like to force continuation with dt=dtmin, set force_dtmin=true")
    end
    return :DtLessThanMin
  end
  if integrator.opts.unstable_check(integrator.dt,integrator.u,integrator.p,integrator.t)
    if integrator.opts.verbose
      warn("Instability detected. Aborting")
    end
    return :Unstable
  end
  if integrator.last_stepfail && !integrator.opts.adaptive
    if integrator.opts.verbose
      warn("Newton steps could not converge and algorithm is not adaptive. Use a lower dt.")
    end
    return :ConvergenceFailure
  end
  return :Default  # or should it be something different?
end

function check_code!(integrator)  # maybe just for internal use, but maybe handy as an API also
  code = check_code(integrator)
  if code != :Default
    integrator.sol = solution_new_retcode(integrator.sol, code)
    postamble!(integrator)
  end
  return code
end

@def ode_exit_conditions begin
  if check_code!(integrator) != :Default
    return integrator.sol
  end
end

@inline done(integrator::ODEIntegrator, _) =
  check_code!(integrator) != :Default

The bonus is that it fixes a possible "bug" in the integrator iterator interface that it was not setting the retcode (not sure if it was intended, though).

@ChrisRackauckas
Copy link
Member

Yeah it would be good to unify this. StochasticDiffEq.jl could probably use the same one.

@tkf
Copy link
Author

tkf commented Apr 12, 2018

Cool. Do you want PRs for this in DiffEqBase, OrdinaryDiffEq, StochasticDiffEq, and DiffEqDocs?

StochasticDiffEq.jl could probably use the same one.

Do you want to define check_code in DiffEqBase? I'm slightly worried that it makes the project hard to tweak, since you need to edit DiffEqBase to change the very core loop of ODE and SDE solvers.

But I see that the current code is effectively identical so I guess it's doable:

@@ -1,7 +1,7 @@
-@def ode_exit_conditions begin
+@def sde_exit_condtions begin
   if integrator.iter > integrator.opts.maxiters
     if integrator.opts.verbose
-      warn("Interrupted. Larger maxiters is needed.")
+      warn("Max Iters Reached. Aborting")
     end
     postamble!(integrator)
     integrator.sol = solution_new_retcode(integrator.sol,:MaxIters)
@@ -23,7 +23,7 @@
     integrator.sol = solution_new_retcode(integrator.sol,:Unstable)
     return integrator.sol
   end
-  if integrator.last_stepfail && !integrator.opts.adaptive
+  if integrator.last_stepfail && !integrator.opts.adaptive # Only false if doubled
     if integrator.opts.verbose
       warn("Newton steps could not converge and algorithm is not adaptive. Use a lower dt.")
     end

@ChrisRackauckas
Copy link
Member

They've been almost identical for like a year now, and I am not seeing why it would change. The interface specifies what they should be doing here, and so they both implement it. It's probably easier to handle together since every change so far has required two duplicate commits.

@tkf
Copy link
Author

tkf commented Apr 13, 2018

I see. That makes sense.

@tkf
Copy link
Author

tkf commented Apr 13, 2018

Some bike-shedding: check_code is OK but it's not super descriptive (I mean, code for what?). So here is a bit of brainstorm:

assess
inspect
judge
audit
check
[assess|...|check_]integrity
[assess|...|check_]irregularity
[assess|...|check_]consistency
[assess|...|check_]condition
check_error
health_check

I like check_integrity and check_error. Anyway, please decide :)

Another question: is returning :Default when no error is detected fine? Should it be :Success or something else than in the return code list like :OK, :Good or :NoError?

@ChrisRackauckas
Copy link
Member

It should be :Success if everything is good. :Default is used only when the method knows it's not doing the proper checks.

tkf added a commit to tkf/DiffEqBase.jl that referenced this issue Apr 13, 2018
tkf added a commit to tkf/OrdinaryDiffEq.jl that referenced this issue Apr 13, 2018
tkf added a commit to tkf/OrdinaryDiffEq.jl that referenced this issue Apr 13, 2018
tkf added a commit to tkf/DiffEqDocs.jl that referenced this issue Apr 13, 2018
tkf added a commit to tkf/OrdinaryDiffEq.jl that referenced this issue Apr 13, 2018
tkf added a commit to tkf/StochasticDiffEq.jl that referenced this issue Apr 13, 2018
tkf added a commit to tkf/OrdinaryDiffEq.jl that referenced this issue Apr 13, 2018
tkf added a commit to tkf/OrdinaryDiffEq.jl that referenced this issue Apr 13, 2018
tkf added a commit to tkf/StochasticDiffEq.jl that referenced this issue Apr 13, 2018
@tkf
Copy link
Author

tkf commented Apr 14, 2018

@ChrisRackauckas Thanks for the merges!

@tkf tkf closed this as completed Apr 14, 2018
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

2 participants