From 47f99834d2631b1fac39941a5dd5cd17677ec525 Mon Sep 17 00:00:00 2001 From: Christopher Rackauckas Date: Wed, 23 Apr 2025 03:49:03 -0400 Subject: [PATCH] Simplify requirements from default polyalg One of the properties the default polyalg wants is not just robustness to the mathematical problem but also robustness to implementation. Certain issues like https://github.com/EnzymeAD/Enzyme.jl/issues/2360#issuecomment-2823335480 have highlighted recently that Backtracking linesearch has a vjp, and then in the scenario that `using Enzyme` is done globally somewhere in the system, the autodiff chooser will prefer Enzyme. That's not necessarily a bad thing, but this leads to odd failures / fixes like https://github.com/SciML/OrdinaryDiffEq.jl/pull/2683 where a seemingly random change to test order fixes things. A fairly common example of how a user can trigger this is that if they do `using Enzyme`, then the forward pass of the default nonlinear solver will now have the Backtracking line search change to using Enzyme reverse mode, which could make a previously solving example fail. Another example is the Broyden with full Jacobian. This has machinery that require `pinv`, which for sparse matrices can be a not well-specified operation and can densify things. So that is not a general operation. It's not even clear that algorithm is a good choice in comparison to NR, since if you're making and `pinv`ing something then why not take a Newton step? Standard Broyden is motivated since it completely avoids the linear algebra but this intermediate step I don't think is really justfied as generally faster than NR or that much more robust, so I think it's an interesting option to maintain but I wouldn't put it in the default path. This then makes the default path hit a few different trust region radius update schemes, which means it's globalized to things that do well, but also only requires forward mode across the board, making things stay simpler. This is a set of defaults that should be less requirements on user `f` functions. We can create other polyalgs that aren't restricted by this requirement, but I think the default should keep it simple in terms of autodiff support. --- src/poly_algs.jl | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/poly_algs.jl b/src/poly_algs.jl index 875bf7e40..143c7328c 100644 --- a/src/poly_algs.jl +++ b/src/poly_algs.jl @@ -40,7 +40,6 @@ function FastShortcutNonlinearPolyalg( else algs = ( NewtonRaphson(; common_kwargs...), - NewtonRaphson(; common_kwargs..., linesearch = BackTracking()), TrustRegion(; common_kwargs...), TrustRegion(; common_kwargs..., radius_update_scheme = RUS.Bastin) ) @@ -52,18 +51,15 @@ function FastShortcutNonlinearPolyalg( if T <: Complex algs = ( SimpleBroyden(), - Broyden(; init_jacobian = Val(:true_jacobian), autodiff), SimpleKlement(), NewtonRaphson(; common_kwargs...) ) else - start_index = u0_len !== nothing ? (u0_len ≤ 25 ? 4 : 1) : 1 + start_index = u0_len !== nothing ? (u0_len ≤ 25 ? 3 : 1) : 1 algs = ( SimpleBroyden(), - Broyden(; init_jacobian = Val(:true_jacobian), autodiff), SimpleKlement(), NewtonRaphson(; common_kwargs...), - NewtonRaphson(; common_kwargs..., linesearch = BackTracking()), TrustRegion(; common_kwargs...), TrustRegion(; common_kwargs..., radius_update_scheme = RUS.Bastin) ) @@ -77,12 +73,11 @@ function FastShortcutNonlinearPolyalg( ) else # TODO: This number requires a bit rigorous testing - start_index = u0_len !== nothing ? (u0_len ≤ 25 ? 4 : 1) : 1 + start_index = u0_len !== nothing ? (u0_len ≤ 25 ? 3 : 1) : 1 algs = ( Broyden(; autodiff), Klement(; linsolve, autodiff), NewtonRaphson(; common_kwargs...), - NewtonRaphson(; common_kwargs..., linesearch = BackTracking()), TrustRegion(; common_kwargs...), TrustRegion(; common_kwargs..., radius_update_scheme = RUS.Bastin) )