Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[OrdinaryDiffEqNonLinearSolve] relax = BackTracking() leads to worse residual in NLNewton #2442

Open
SouthEndMusic opened this issue Aug 29, 2024 · 1 comment · May be fixed by #2443
Open
Labels

Comments

@SouthEndMusic
Copy link
Member

SouthEndMusic commented Aug 29, 2024

I observed some weird behavior in my ODE solve with NLNewton(; relax = BackTracking()) where the result is obviously wrong. After some digging I wrote a wrapper of BackTracking where I reject the relaxation when the residu becomes worse w.r.t. to the residu in the full newton step:

@kwdef struct MonitoredBackTracking{B, V}
    linesearch::B = BackTracking()
    dz_tmp::V = []
    z_tmp::V = []
end

"""
MonitoredBackTracing is a thin wrapper of BackTracking, making sure that
the BackTracking relaxation is rejected if it results in a residual increase
"""
function OrdinaryDiffEq.relax!(
    dz,
    nlsolver::AbstractNLSolver,
    integrator::DEIntegrator,
    f,
    linesearch::MonitoredBackTracking,
)
    (; linesearch, dz_tmp, z_tmp) = linesearch

    # Store step before relaxation
    @. dz_tmp = dz

    # Apply relaxation and measure the residu change
    @. z_tmp = nlsolver.z + dz
    resid_before = resid(z_tmp, integrator, nlsolver, f) # The function resid mimicks this one: https://github.com/SciML/OrdinaryDiffEq.jl/blob/e3af6419b02810ea3b762ab0511cda0e4bcf245c/lib/OrdinaryDiffEqNonlinearSolve/src/newton.jl#L426
    relax!(dz, nlsolver, integrator, f, linesearch)
    @. z_tmp = nlsolver.z + dz
    resid_after = resid(z_tmp, integrator, nlsolver, f)

    # If the residu increased due to the relaxation, reject it
    if resid_after > resid_before
        @. dz = dz_tmp
    end
end

I found out that many other linesearch methods from LineSearches.jl do a test whether the line is in a descending direction, but BackTracking doesn't, so maybe BackTracking works with that assumption which is sometimes wrong.

But what I find most strange is that these wrong relaxed steps get accepted in all layers: BackTracking, NLNewton, QNDF.

@SouthEndMusic SouthEndMusic changed the title relax = BackTracking() leads to worse residu in relax = BackTracking() leads to worse residu in NLNewton Aug 29, 2024
@SouthEndMusic SouthEndMusic changed the title relax = BackTracking() leads to worse residu in NLNewton relax = BackTracking() leads to worse residu in NLNewton Aug 29, 2024
@SouthEndMusic
Copy link
Member Author

Maybe alpha = 1 is just never considered?

@SouthEndMusic SouthEndMusic changed the title relax = BackTracking() leads to worse residu in NLNewton relax = BackTracking() leads to worse residual in NLNewton Aug 29, 2024
SouthEndMusic added a commit to Deltares/Ribasim that referenced this issue Aug 29, 2024
`BackTracking` as relaxation is now enabled again, with a thin wrapper
to reject it when the residual gets worse. Upstream issue:

SciML/OrdinaryDiffEq.jl#2442
@SouthEndMusic SouthEndMusic changed the title relax = BackTracking() leads to worse residual in NLNewton [OrdinaryDiffEqNonLinearSolve] relax = BackTracking() leads to worse residual in NLNewton Aug 29, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant