New Root Solver: ITP Method by duncanam · Pull Request #544 · argmin-rs/argmin

duncanam · 2024-12-01T20:50:47Z

Intro

Hello! I noticed that this lovely crate does not have an implementation of the ITP method root solver, which is, quoting Wikipedia:

"The ITP method, short for Interpolate Truncate and Project, is the first root-finding algorithm that achieves the superlinear convergence of the secant method while retaining the optimal worst-case performance of the bisection method. It is also the first method with guaranteed average performance strictly better than the bisection method under any continuous distribution. In practice it performs better than traditional interpolation and hybrid based strategies (Brent's Method, Ridders, Illinois), since it not only converges super-linearly over well behaved functions but also guarantees fast performance under ill-behaved functions where interpolations fail."

Practically, it is faster than BrentRoot due to fewer function evaluations. I thought this might be a handy contribution towards argmin, since I didn't see too many implementations of this online (There's the itp package in R, and in Julia I think it exists in Roots.jl. I also found it located within the Rust crate kurbo here, but this was tuned specifically for curve fitting instead of generic root solving. I templated this implementation based off of brentroot.rs.

Verification

I used the quadratic test from brentroot.rs, and it achieved a solution in one iteration faster. However, there's an additional function evaluation that occurs to satisfy the argmin iteration state, so currently comes in equal in terms of function evaluations with BrentRoot for that example specifically.

Additionally, I added a test against the polynomial shown in the example on Wikipedia, and stepping through in a debugger I was able to verify that a and b bounds on the bracket (of the solver) indeed match what is in that table there.

Let me know what else should be changed, as I'm new to this community. Thanks! 😃

duncanam · 2024-12-01T20:53:51Z

crates/argmin/src/solver/itp/itp_method.rs

+            max,
+            tol,
+            // kappa1, suggested from paper
+            float!(0.2) / (max - min),


Wondering, maybe we want to have this function return Result and verify max - min cannot be zero so we don't panic here. Not sure though, maybe not worth it right now.

This sounds like a good addition!

duncanam · 2024-12-01T20:54:25Z

crates/argmin/src/solver/itp/itp_method.rs

+            let sol = (self.a + self.b) * float!(0.5);
+            // TODO: This function evaluation serves no purpose other than to serve argmin's cost
+            // method on the state. It feels wasteful.
+            let f_sol = problem.cost(&sol)?;


This bothers me, but maybe someone smarter than me might know a clever way to not do this.

I agree. Do I understand correctly that the algorithm itself does not require to compute the final cost function value? It is only required here because otherwise the cost function value would not be related to the final solution?

Yes- the solution architecture sets it up such that the subsequent evaluation would satisfy the tolerance, rather than needing to evaluate and check success criteria. We could, in theory, return maybe a theoretical value given this information, but it seems misleading.

I agree that this is wasteful, but I see no other way right now to return a cost function value without computing it once more. You could think about a flag which turns off the computation, thus not returning any value (I think this should work).

codecov-commenter · 2024-12-07T21:10:20Z

Codecov Report

Attention: Patch coverage is 95.30516% with 10 lines in your changes missing coverage. Please review.

Project coverage is 91.94%. Comparing base (c7673ef) to head (d74b21c).

Files with missing lines	Patch %	Lines
crates/argmin/src/solver/itp/itp_method.rs	95.30%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
+ Coverage   91.92%   91.94%   +0.01%     
==========================================
  Files         177      178       +1     
  Lines       23724    23940     +216     
==========================================
+ Hits        21808    22011     +203     
- Misses       1916     1929      +13

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

duncanam · 2024-12-17T07:53:53Z

@stefan-k Does this seem like a worthwhile addition? If so, I did take a peek at CI here and some of these seem like flakes- I could totally be misreading, though.

stefan-k · 2024-12-17T09:52:47Z

Hi @duncanam , this is definitely a worthwhile addition, thank you! Unfortunately it will take a bit until I can give a useful review, hopefully around the upcoming holidays. Apologies for being so unresponsive recently :/

duncanam · 2024-12-17T14:01:46Z

Hi @duncanam , this is definitely a worthwhile addition, thank you! Unfortunately it will take a bit until I can give a useful review, hopefully around the upcoming holidays. Apologies for being so unresponsive recently :/

No worries! Have a good holiday season!

stefan-k

Thank you for this PR! This is a very valuable addition and looks pretty good to me. :) I only have a few minor points to discuss. Regarding the math I'll have to trust you here as I currently am not able to dive into this in detail.

crates/argmin/src/solver/itp/itp_method.rs

stefan-k · 2025-01-03T09:09:54Z

crates/argmin/src/solver/itp/itp_method.rs

+            tol,
+            kappa1,
+            kappa2,
+            n0,
+            a: min,
+            b: max,


Question out of curiosity: Are there any sane defaults for these parameters (ideally something mentioned in the paper)?

That was my goal with the defaults method in this file- is there a better way to notate that idiomatically? I've seen some packages use default(), but the "classmethod" style of using from_ seems to also be clear.

The way this is typically solved in argmin is by the new method acting more or less like a default method. Anything that has reasonable defaults is set there. Then there are with_* methods which allow for overwriting certain parameters. You can see this in action for instance here.

stefan-k · 2025-01-03T09:13:16Z

crates/argmin/src/solver/itp/itp_method.rs

+            let sol = (self.a + self.b) * float!(0.5);
+            // TODO: This function evaluation serves no purpose other than to serve argmin's cost
+            // method on the state. It feels wasteful.
+            let f_sol = problem.cost(&sol)?;


I agree. Do I understand correctly that the algorithm itself does not require to compute the final cost function value? It is only required here because otherwise the cost function value would not be related to the final solution?

stefan-k · 2025-01-03T09:16:03Z

I'll try to fix the CI problems in another PR.

stefan-k · 2025-03-04T13:05:57Z

crates/argmin/src/solver/itp/itp_method.rs

+                state
+                    .terminate_with(TerminationReason::SolverConverged)
+                    .param(sol)
+                    .cost(f_sol.abs()),


Is the .abs() necessary here?

stefan-k · 2025-03-04T13:06:21Z

crates/argmin/src/solver/itp/itp_method.rs

+            ));
+        }
+
+        Ok((state.param(self.b).cost(self.fb.abs()), None))


Same question here regarding .abs()

duncanam · 2025-12-09T18:21:48Z

I'm sorry this has taken so long. Let me try and pull this across the finish line.

duncanam added 2 commits December 1, 2024 11:26

Typo fix

4471858

Add ITP method root solver

d74b21c

duncanam commented Dec 1, 2024

View reviewed changes

stefan-k reviewed Jan 3, 2025

View reviewed changes

duncanam added 2 commits January 17, 2025 07:08

fixup! moving some value checks into the init

ebb14ec

!fixup adding check for divide by zero in init

9e246fa

stefan-k reviewed Mar 4, 2025

View reviewed changes

Uh oh!

Conversation

duncanam commented Dec 1, 2024

Intro

Verification

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Dec 7, 2024

Codecov Report

Uh oh!

duncanam commented Dec 17, 2024

Uh oh!

stefan-k commented Dec 17, 2024

Uh oh!

duncanam commented Dec 17, 2024

Uh oh!

stefan-k left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefan-k commented Jan 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duncanam commented Dec 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants