Add Halley's method via descent API #404
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #404 +/- ##
==========================================
- Coverage 86.45% 77.45% -9.01%
==========================================
Files 47 49 +2
Lines 2872 2918 +46
==========================================
- Hits 2483 2260 -223
- Misses 389 658 +269 ☔ View full report in Codecov by Sentry. |
|
@avik-pal Support for in-place I think there isn't an eqivalent for |
Yes mark it as |
|
I am trying to add this as a block in
|
|
Yeah ReTestItems doesn't yet work with VSCode Test Runner. In a julia REPL activate the test using TestEnv then do using ReTestItems
ReTestItems.runtests(<path to file or test directory>; name="<name of the testitem>") |
|
Ok I added it to this file near |
|
@avik-pal Do you have an out-of-place version of 23 problems? When testing with |
|
Don't the tests in simplenonlinearsolve use the oop version? |
|
So currently, the tests at https://github.com/SciML/SimpleNonlinearSolve.jl/blob/main/test/core/23_test_problems_tests.jl doesn't include Halley's method, and when I tried to add Halley in the same way as Newton, I get the error above. And the file at https://github.com/SciML/DiffEqProblemLibrary.jl/blob/master/lib/NonlinearProblemLibrary/src/NonlinearProblemLibrary.jl doesn't have out-of-place |
|
Ok so I manually ran |
|
Should be fine now, @avik-pal could you review that again? |
|
Handle the test failures, the |
|
Ok, tried to fix the precompilation issue and added compat |
|
Now all fails are unrelated |
|
Are you sure? There are no failures on master. Some weird interaction of Taylor diff with zygote? |
|
hmm, that's weird. The Zygote stuff in TaylorDiff doesn't have type piracy so shouldn't have weird interactions |
|
@tansongchen TaylorDiff is definitely causing problems. See #441 |
tansongchen
force-pushed
the
master
branch
4 times, most recently
from
00bded1 to
198cd4d
Compare
|
@avik-pal Could you please review this again? Comparing https://github.com/SciML/NonlinearSolve.jl/actions/runs/11667334463/job/32484400696 and https://github.com/SciML/NonlinearSolve.jl/actions/runs/11667812262/job/32486004697?pr=404 , I think this is not introducing new failures |
| @@ -25,6 +25,7 @@ PrecompileTools = "aea7be01-6a6a-4083-8856-8a6e6704d82a" | |||
| Preferences = "21216c6a-2e73-6563-6e65-726566657250" | |||
| Reexport = "189a3867-3050-52da-a836-e630ba90ab69" | |||
| SciMLBase = "0bca4576-84f4-4d90-8ffe-ffa030f20462" | |||
| SciMLJacobianOperators = "19f34311-ddf3-4b8b-af20-060888a46c0e" | |||
There was a problem hiding this comment.
Not sure why this comes in, I think that got in when I dev the libs. can be removed
| using NonlinearSolveBase: HalleyDescentCache | ||
| import NonlinearSolveBase: evaluate_hvvp | ||
| using TaylorDiff: derivative, derivative! | ||
| using FastClosures: @closure | ||
|
|
||
| function evaluate_hvvp( |
There was a problem hiding this comment.
| using NonlinearSolveBase: HalleyDescentCache | |
| import NonlinearSolveBase: evaluate_hvvp | |
| using TaylorDiff: derivative, derivative! | |
| using FastClosures: @closure | |
| function evaluate_hvvp( | |
| using NonlinearSolveBase: NonlinearSolveBase, HalleyDescentCache | |
| using TaylorDiff: derivative, derivative! | |
| using FastClosures: @closure | |
| function NonlinearSolveBase.evaluate_hvvp( |
style nit
| """ | ||
| Halley(; concrete_jac = nothing, linsolve = nothing, linesearch = missing, | ||
| autodiff = nothing) | ||
|
|
||
| An experimental Halley's method implementation. Improves the convergence rate of Newton's method by using second-order derivative information to correct the descent direction. | ||
|
|
||
| Currently depends on TaylorDiff.jl to handle the correction terms, | ||
| might have more general implementation in the future. | ||
| """ | ||
| function Halley(; concrete_jac = nothing, linsolve = nothing, | ||
| linesearch = missing, autodiff = nothing) | ||
| return GeneralizedFirstOrderAlgorithm(; | ||
| concrete_jac, name = :Halley, linesearch, | ||
| descent = HalleyDescent(; linsolve), autodiff) | ||
| end |
There was a problem hiding this comment.
Not really a First Order method, we might want an additional split cc @ChrisRackauckas
Add Halley's method by computing Hessian-vector-vector product with TaylorDiff.jl in$O(n)$ time. Note that this isn't structured as something like "HessianCache", but instead structured as a descent method, which can be viewed as "adding some correction term to the Newton's method". This is mainly because Hessian, either implicitly or explicitly, isn't likely to be used elsewhere.
Tests haven't been added because TaylorDiff.jl should be added to an extension. Once that is done, it is straightforward to add tests.
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.