Skip to content

Allow generic solver to be passed to Newton method #1176

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rsenne
Copy link

@rsenne rsenne commented Jul 10, 2025

This PR is in reference to issue #1170. To solve this I have edited the interface to allow a user to pass a generic solver. This in theory would allow one to use any matrix type given the user has that package loaded and there exists a solver designed for that matrix type on board. This also allows one to write a custom solver (i.e., problem specific factorization, strategies for solving) to circumvent the default behavior. I have included some examples of this in testing including:

  1. Different factorizations (would allow users to circumvent the default PositiveFactorizations.jl implementation of cholesky)
  2. Usage of a non-originally supported matrix type (StaticArray)
  3. Test type preservation of Hessian with TwiceDifferentiable object using BlockArrays.jl
  4. A more realistic example of a non-trivial use case (block-tridiagonal hessian) this implementation is not totally optimized as it is a proof of principle

Let me know if there are any other tests you would like me to include or changes to the documentation.

Copy link
Contributor

Benchmark Results

master 330ffb4... master / 330ffb4...
multivariate/solvers/first_order/AdaMax 0.652 ± 0.0083 ms 0.647 ± 0.008 ms 1.01 ± 0.018
multivariate/solvers/first_order/Adam 0.652 ± 0.008 ms 0.647 ± 0.008 ms 1.01 ± 0.018
multivariate/solvers/first_order/BFGS 0.224 ± 0.0037 ms 0.234 ± 0.0069 ms 0.955 ± 0.032
multivariate/solvers/first_order/ConjugateGradient 0.0481 ± 0.00054 ms 0.0502 ± 0.00064 ms 0.958 ± 0.016
multivariate/solvers/first_order/GradientDescent 1.71 ± 0.016 ms 1.79 ± 0.007 ms 0.956 ± 0.0098
multivariate/solvers/first_order/LBFGS 0.219 ± 0.0037 ms 0.228 ± 0.0051 ms 0.957 ± 0.027
multivariate/solvers/first_order/MomentumGradientDescent 2.51 ± 0.018 ms 2.59 ± 0.011 ms 0.968 ± 0.008
multivariate/solvers/first_order/NGMRES 0.553 ± 0.011 ms 0.554 ± 0.011 ms 0.999 ± 0.028
time_to_load 0.529 ± 0.0013 s 0.53 ± 0.0063 s 0.999 ± 0.012

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@pkofod
Copy link
Member

pkofod commented Jul 17, 2025

Thanks, I'll review :)

Copy link

codecov bot commented Jul 17, 2025

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.69%. Comparing base (6850998) to head (330ffb4).

Files with missing lines Patch % Lines
src/multivariate/solvers/second_order/newton.jl 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1176      +/-   ##
==========================================
- Coverage   85.70%   85.69%   -0.02%     
==========================================
  Files          46       46              
  Lines        3596     3593       -3     
==========================================
- Hits         3082     3079       -3     
  Misses        514      514              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants