search

SciML / OrdinaryDiffEq.jl

High performance ordinary differential equation (ODE) and differential-algebraic equation (DAE) solvers, including neural ordinary differential equations (neural ODEs) and scientific machine learning (SciML)
https://diffeq.sciml.ai/latest/
Other
521 stars 198 forks source link

Fixing dolinsolve(), so that it will pass u,p,t to the precs() function #2254

Closed Leebre closed 1 week ago

Leebre commented 2 weeks ago

Fixing dolinsolve(), so that it will pass u,p,t to the precs() function properly during the solve. Previously, these were always being passed as ::Nothing.

Additional context

The change was originally proposed by Oscar Smith, but was previously cancelled. I would like to request this be reviewed again. I have tested with and without the change. Without it, the solver never passes u,p,t to precs() - it only ever passes ::Nothing during the solve. With this fix, there are no errors and u,p,t get passed during the solve, which means they can be used for updating the preconditioner.

Leebre commented 2 weeks ago

@oscardssmith FYI: I am re-submitting your fix for dolinsolve(), with regards to passing variables to the precs() function. I have tested with the fix and without. Without the fix, u,p,t never get passed to precs(), which is surely a bug.

oscardssmith commented 2 weeks ago

The fix as I wrote it at least is not correct (since the Rosenbrock methods intentionally pass it in some cases but don't for ohters).

Leebre commented 2 weeks ago

@oscardssmith ok, thanks for your input on this. I think the issue needs to be reviewed again, as the current code definitely doesn't seem to be working correctly, as far as passing these variables to precs(). Perhaps something could be done differently for the specific case of the Rosenbrock methods? Your fix seems to work very well for me, for using a preconditioner with kencarp4.

ChrisRackauckas commented 2 weeks ago

This isn't correct? It's documented why this is the case? I thought we had discussed on Slack and I pointed to the part of the documentation which describes that the interface requires that you can handle a nothing dispatch which is used to defining the type.

oscardssmith commented 2 weeks ago

The problem is that none of the solvers other than the Rosenbrocks ever pass u,p,t in, and the Rosenbrock solvers sometimes do and sometimes don't. It appears that the code is wrong, but I don't really understand how...

Leebre commented 2 weeks ago

Adding the nothing dispatch avoids the error I was seeing before. However, even with that, the solver still never passes u, p, t to the precs() function during the solve. Therefore, u, p, t can't be used to reconstruct the preconditioner.

oscardssmith commented 1 week ago

closing in favor of https://github.com/SciML/OrdinaryDiffEq.jl/pull/2247