Simplify requirements from default polyalg #583
Merged
+2
−7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
One of the properties the default polyalg wants is not just robustness to the mathematical problem but also robustness to implementation. Certain issues like EnzymeAD/Enzyme.jl#2360 (comment) 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 SciML/OrdinaryDiffEq.jl#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.
|
@avik-pal https://github.com/SciML/NonlinearSolve.jl/actions/runs/14612790929/job/40994078714?pr=583#step:5:1270 this is randomly showing up and not actually related right? I'm removing this method from the default method, but now it fails in the test 😅 |
|
Noticed it was failing a bit ago in SciML/DiffEqBase.jl#1141 so it's just not related. |
|
oh it was SciML/DiffEqProblemLibrary.jl#138 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One of the properties the default polyalg wants is not just robustness to the mathematical problem but also robustness to implementation. Certain issues like EnzymeAD/Enzyme.jl#2360 (comment) have highlighted recently that Backtracking linesearch has a vjp, and then in the scenario that
using Enzymeis 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 SciML/OrdinaryDiffEq.jl#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 dousing 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 andpinving 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
ffunctions. 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.