Skip to content

Conversation

@Luis-Varona
Copy link

The Shubert-Piyavskii method optimizes a univariate, Lipschitz continuous function inside a specified interval. Given an accurate Lipschitz constant, it is guaranteed to come within an arbitrary epsilon of the true global minimum. Unlike most other deterministic global optimizations algorithm (e.g., golden-section search), it is not restricted to unimodal functions; moreover, it does not require an initial guess. It deterministically samples points from ever-narrowing subintervals of the search space until the sampled best point is sufficiently close to the lower bound given by the Lipschitz constant.

I have added an implementation of an SP solver in crates/argmin/src/solver/shubertpiyavskii/, modifying crates/argmin/src/lib.rs and crates/argmin/src/solver/mod.rs accordingly. Additionally, I have added an example run of the solver to examples/shubertpiyavskii/.

@Luis-Varona
Copy link
Author

Just added a minor bugfix to one of the doctests (didn't affect the actual implementation, though)--squashed into the same commit to avoid clutter.

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2025

Codecov Report

❌ Patch coverage is 95.47414% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.17%. Comparing base (b35808a) to head (5d75b8d).
⚠️ Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
crates/argmin/src/solver/shubertpiyavskii/mod.rs 95.47% 21 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #588      +/-   ##
==========================================
+ Coverage   92.13%   92.17%   +0.04%     
==========================================
  Files         177      178       +1     
  Lines       23672    24139     +467     
==========================================
+ Hits        21810    22250     +440     
- Misses       1862     1889      +27     

☔ 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.

@Luis-Varona Luis-Varona force-pushed the 587-shubertpiyavskii branch from 90d8204 to c99ead9 Compare April 20, 2025 21:17
@Luis-Varona
Copy link
Author

Luis-Varona commented Apr 20, 2025

One last bugfix--a spelling correction ("arbitraily" -> "arbitrarily") in the examples file and making the PartialOrd impl for SearchInterval more idiomatic in the main solver implementation. Should pass all CI tests now! 🙂

@Luis-Varona Luis-Varona force-pushed the 587-shubertpiyavskii branch from c99ead9 to 4bf9bf2 Compare April 20, 2025 21:21
The Shubert-Piyavskii method optimizes a univariate, Lipschitz continuous function inside a specified interval. Given an accurate Lipschitz constant, it is guaranteed to come within an arbitrary epsilon of the true global minimum. Unlike most other deterministic global optimizations algorithm (e.g., golden-section search), it is not restricted to unimodal functions; moreover, it does not require an initial guess. It deterministically samples points from ever-narrowing subintervals of the search space until the sampled best point is sufficiently close to the lower bound given by the Lipschitz constant.

I have added an implementation of an SP solver in crates/argmin/src/solver/shubertpiyavskii/, modifying crates/argmin/src/lib.rs and crates/argmin/src/solver/mod.rs accordingly. Additionally, I have added an example run of the solver to examples/shubertpiyavskii/.
@Luis-Varona Luis-Varona force-pushed the 587-shubertpiyavskii branch from 4bf9bf2 to 5d75b8d Compare April 21, 2025 07:21
@Luis-Varona
Copy link
Author

Luis-Varona commented Apr 21, 2025

One last fix; code and doctests were all working, but clippy got mad at something non-idiomatic in a match branch, hehe. Now it should be 100% ready, pending your approval @stefan-k :) (No rush!)

@Luis-Varona
Copy link
Author

Hi @stefan-k! Definitely no rush, but I just wanted to ask if you still planned to get around to reviewing this at some point in the future (doesn't matter when!) 🙂 I also plan to make a PR sometime soon for Differential Evolution as per my conversation with @steinhauserc in #499, if you think that's a good idea.

@stefan-k
Copy link
Member

Hi @stefan-k! Definitely no rush, but I just wanted to ask if you still planned to get around to reviewing this at some point in the future (doesn't matter when!) 🙂

Sincere apologies, I totally missed this! Thanks for your contribution, I'll try to do a review within the coming days (hopefully even today). Please ping me again should I not get back to you by Sunday.

I also plan to make a PR sometime soon for Differential Evolution as per my conversation with @steinhauserc in #499, if you think that's a good idea.

Absolutely yes! :)


impl<F: ArgminFloat> PartialEq for SearchInterval<F> {
fn eq(&self, other: &Self) -> bool {
self.lower_bound == other.lower_bound
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This compares two floats for equivalence which is probably not what you want here. I assume you want to check if those two values are close to each other within a given tolerance. I would avoid hiding this operation within an PartialEq implementation, instead make it explicit in the code (and potentially even make the tolerance configurable with a sane default).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This compares two floats for equivalence which is probably not what you want here. I assume you want to check if those two values are close to each other within a given tolerance. I would avoid hiding this operation within an PartialEq implementation, instead make it explicit in the code (and potentially even make the tolerance configurable with a sane default).

I implemented this to facilitate the Ord/PartialOrdimpls for SearchInterval (as I described in another review response). On this particular issue, I'm more inclined to agree with you and extract the logic into the Ord/PartialOrd impls themselves?

Given my explanation for the Ord/PartialOrd impls, what do you think is most appropriate?

Comment on lines +86 to +96
impl<F: ArgminFloat> Ord for SearchInterval<F> {
fn cmp(&self, other: &Self) -> Ordering {
other.lower_bound.partial_cmp(&self.lower_bound).unwrap()
}
}

impl<F: ArgminFloat> PartialOrd for SearchInterval<F> {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain your reasoning behind implementing Ord/PartialOrd for SearchInterval? Thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain your reasoning behind implementing Ord/PartialOrd for SearchInterval? Thanks!

Hi @stefan-k - I did this so that we can use (for some F: ArgminFloat the BinaryHeap<SearchInterval<F>> needed to process the search intervals by those with smaller lower bounds first. To my understanding, this is the easiest way to implement a priority queue for the search intervals, but definitely let me know if you have a better implementation alternative?

@Luis-Varona
Copy link
Author

Hi, @stefan-k—I'll try to respond to your review over the weekend :)

@Luis-Varona
Copy link
Author

Hi @stefan-k, sorry for the delay. I'll look over and respond to your comments later today for sure!

@Luis-Varona
Copy link
Author

@stefan-k, I just responded :) TL;DR: The Ord/PartialOrd impls are needed so we can use a binary heap of SearchIntervals, but you might be right w.r.t. the Eq/PartialEq impls not being necessary. Definitely let me know what you think!

@Luis-Varona
Copy link
Author

Hi @stefan-k, just following up again.

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.

3 participants