SciML / NonlinearSolve.jl

High-performance and differentiation-enabled nonlinear solvers (Newton methods), bracketed rootfinding (bisection, Falsi), with sparsity and Newton-Krylov support.
https://docs.sciml.ai/NonlinearSolve/stable/
MIT License
216 stars 39 forks source link

Add Halley's method via descent API #404

Open tansongchen opened 2 months ago

tansongchen commented 2 months ago

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

Additional context

Add any other context about the problem here.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 66.66667% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 77.45%. Comparing base (7deb1ed) to head (92264c0).

Files Patch % Lines
src/descent/halley.jl 64.58% 17 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tansongchen commented 2 months ago

@avik-pal Support for in-place f and tests are added. Let me know if you have any questions!

I think there isn't an eqivalent for Fix2 that fixes the third argument, which is needed by evaluating the derivatives given p. Is it ok to use a closure?

avik-pal commented 2 months ago

I think there isn't an eqivalent for Fix2 that fixes the third argument, which is needed by evaluating the derivatives given p. Is it ok to use a closure?

Yes mark it as @closure and do it

tansongchen commented 2 months ago

I am trying to add this as a block in 23_test_problems_tests.jl so I can make use of these problems. Do you know how to run a @testitem independently? With the vscode built-in test runner, I am getting the following error:

image

avik-pal commented 2 months ago

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>")
tansongchen commented 2 months ago

Ok I added it to this file near NewtonRaphson. Some cases haven't work mainly due to limitations of TaylorDiff.jl

tansongchen commented 1 month ago

@avik-pal Do you have an out-of-place version of 23 problems? When testing with SimpleHallely, I hit

12:48:34 | START (1/1) test item "SimpleHalley" at test/core/23_test_problems_tests.jl:53
┌ Error: ErrorException("SimpleHalley currently only supports out-of-place nonlinear problems")
└ @ Main.var"##RobustnessTesting#247" ~/Public/SimpleNonlinearSolve.jl/test/core/23_test_problems_tests.jl:28
avik-pal commented 1 month ago

Don't the tests in simplenonlinearsolve use the oop version?

tansongchen commented 1 month ago

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 f's

tansongchen commented 1 month ago

Ok so I manually ran [1, 5, 8, 15, 16] against SimpleHalley, and 1, 5, 15, 16 also failed. 8 is due to an error on TaylorDiff.jl and has been fixed. I need to release a new version of TaylorDiff to make the CIs pass though.

tansongchen commented 1 month ago

Should be fine now, @avik-pal could you review that again?

avik-pal commented 1 month ago

Handle the test failures, the Misc ones are real

tansongchen commented 1 month ago

Ok, tried to fix the precompilation issue and added compat

tansongchen commented 1 month ago

Now all fails are unrelated

avik-pal commented 1 month ago

Are you sure? There are no failures on master. Some weird interaction of Taylor diff with zygote?

tansongchen commented 1 month ago

hmm, that's weird. The Zygote stuff in TaylorDiff doesn't have type piracy so shouldn't have weird interactions

avik-pal commented 1 month ago

@tansongchen TaylorDiff is definitely causing problems. See https://github.com/SciML/NonlinearSolve.jl/pull/441