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

[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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant