Skip to content

Commit 4b3cafd

Browse files
do_newJW: refactorize W on the linear path when γdt drifts (#3934)
For islinearfunction problems do_newJW returned (false, false) after the first step, assuming a constant J means W never changes — but W = J - M/(γdt) depends on the step size, so concrete-A linear solvers kept the factorization from the first step's dt forever. Combined with WOperator._concrete_form being stale for operator Jacobians (SciML/SciMLOperators.jl#408), implicit solvers on SplitODEProblem(MatrixOperator(A), f) with LU/KLU returned garbage with retcode Success in ~2 steps (#3933). Keep new_jac = false on the linear path (J really is constant) but apply the standard W_γdt cutoff test for new_W, matching the DAE branch below. Adds an end-to-end regression test (KenCarp3/4/5 + LU on a 2x2 split operator problem, non-split TRBDF2, and the always-working Krylov path); requires SciMLOperators >= 1.24.4 (compat bumped) for the companion fix, so CI will resolve only once that releases. Fixes #3933 together with SciML/SciMLOperators.jl#408. Co-authored-by: ChrisRackauckas-Claude <accounts@chrisrackauckas.com>
1 parent 754b504 commit 4b3cafd

3 files changed

Lines changed: 56 additions & 1 deletion

File tree

lib/OrdinaryDiffEqDifferentiation/src/derivative_utils.jl

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,17 @@ function do_newJW(integrator, alg, nlsolver, repeat_step)::NTuple{2, Bool}
509509
integrator.iter <= 1 && return true, true # at least one JW eval at the start
510510
repeat_step && return false, false
511511
islin, _ = islinearfunction(integrator)
512-
islin && return false, false # no further JW eval when it's linear
512+
if islin
513+
# J is constant for a linear function, but W = J - M/(γdt) still depends
514+
# on γdt: W must be rebuilt/refactorized when the step size has drifted
515+
# past the cutoff, otherwise concrete-A linear solvers keep a stale
516+
# factorization from the first step's (possibly tiny) dt.
517+
isnewton(nlsolver) || return false, true
518+
W_iγdt = inv(nlsolver.cache.W_γdt)
519+
iγdt = inv(nlsolver.γ * integrator.dt)
520+
smallstepchange = abs(iγdt / W_iγdt - 1) <= get_new_W_γdt_cutoff(nlsolver)
521+
return false, !smallstepchange
522+
end
513523
!integrator.opts.adaptive && return true, true # Not adaptive will always refactorize
514524
errorfail = OrdinaryDiffEqCore.get_EEst(integrator) > one(OrdinaryDiffEqCore.get_EEst(integrator))
515525
# TODO: add `isJcurrent` support for Rosenbrock solvers

lib/OrdinaryDiffEqDifferentiation/test/runtests.jl

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ if TEST_GROUP ∉ ("QA", "Sparse", "ModelingToolkit")
3535
@time @safetestset "Differentiation Trait Tests" include("differentiation_traits_tests.jl")
3636
@time @safetestset "Autodiff Error Tests" include("autodiff_error_tests.jl")
3737
@time @safetestset "No Jac Tests" include("nojac_tests.jl")
38+
@time @safetestset "Stale W Linear Operator Tests" include("stale_w_linear_operator_tests.jl")
3839
end
3940

4041
# Run sparse tests (separate environment due to ComponentArrays dep conflicts)
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
# Regression tests for SciML/OrdinaryDiffEq.jl#3933: implicit solvers with an
2+
# AbstractSciMLOperator Jacobian (islinearfunction path) and a concrete-matrix
3+
# linear solver silently returned garbage, because do_newJW never requested a W
4+
# refactorization for linear operators (W = J - M/(γdt) changes with dt even
5+
# when J is constant). Requires SciMLOperators >= 1.24.4 for the companion fix
6+
# that rebuilds WOperator._concrete_form on convert for operator Jacobians.
7+
using OrdinaryDiffEqSDIRK, OrdinaryDiffEqDifferentiation, LinearSolve, SciMLOperators
8+
using SciMLBase, LinearAlgebra, Test
9+
10+
@testset "stale W with linear operator f and concrete-A linsolve" begin
11+
A = [-2.0 1.0; 1.0 -2.0]
12+
B = [0.0 0.5; -0.5 0.0]
13+
u0 = [1.0, 0.5]
14+
tspan = (0.0, 1.0)
15+
uref = exp((A + B) * tspan[2]) * u0
16+
17+
f2! = (du, u, p, t) -> mul!(du, B, u)
18+
prob_split = SplitODEProblem(MatrixOperator(A), f2!, u0, tspan)
19+
20+
@testset "$(nameof(typeof(alg))) split, LU" for alg in (
21+
KenCarp3(linsolve = LUFactorization()),
22+
KenCarp4(linsolve = LUFactorization()),
23+
KenCarp5(linsolve = LUFactorization()),
24+
)
25+
sol = solve(prob_split, alg; abstol = 1.0e-8, reltol = 1.0e-8)
26+
@test SciMLBase.successful_retcode(sol)
27+
@test norm(sol.u[end] - uref) / norm(uref) < 1.0e-3
28+
# the broken path accepted the whole interval in ~2 giant steps
29+
@test length(sol.t) > 5
30+
end
31+
32+
# Non-split linear-operator problem hits the same do_newJW islin branch
33+
prob_lin = ODEProblem(ODEFunction(MatrixOperator(A + B)), u0, tspan)
34+
sol = solve(prob_lin, TRBDF2(linsolve = LUFactorization()); abstol = 1.0e-8, reltol = 1.0e-8)
35+
@test SciMLBase.successful_retcode(sol)
36+
@test norm(sol.u[end] - uref) / norm(uref) < 1.0e-3
37+
38+
# The Krylov path was always correct; make sure it stays that way
39+
sol_krylov = solve(
40+
prob_split, KenCarp4(linsolve = KrylovJL_GMRES());
41+
abstol = 1.0e-8, reltol = 1.0e-8
42+
)
43+
@test norm(sol_krylov.u[end] - uref) / norm(uref) < 1.0e-3
44+
end

0 commit comments

Comments
 (0)