Skip to content
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

add relative tolerance to BranchAndBoundEnclosure #179

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

Conversation

aplavin
Copy link

@aplavin aplavin commented May 29, 2024

Thanks for the package, it's already very useful and convenient!

Here, I suggest to add an rtol argument, so that one can easily specify the target tolerance independent on the scale of values in a problem.

If this addition is welcome, we can discuss the best way to backwards compatibility: either keep tol and not rename it to atol, or add a tol alias for atol.

Copy link
Member

@schillic schillic left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal. I do not fully understand. Can you give an example?

src/branchandbound.jl Outdated Show resolved Hide resolved
@aplavin
Copy link
Author

aplavin commented May 29, 2024

Basically, this PR makes it possible to run enclose for functions with very different scales (think f(x) and 1e10 * f(x)) with the same options: just specify rtol!

Currently, tol is absolute, and tol = 1e-3 is a very big tolerance for f(x) = 1e-10 * x (will stop on the first iteration), but is a very small tolerance for f(x) = 1e10 * x (will only stop at maxdepth).

@schillic
Copy link
Member

Okay, makes sense now. Some comments:

  • We need to make a breaking release in any case because the behavior will change. I do not mind that at all.
  • The old behavior is atol=1e-3, and now atol=1e-6. I do not have an opinion which one to choose.
  • I prefer to be clear, i.e., to rename the field to atol. We could offer a constructor with a tol alias for atol, as you suggest, but I do not think this is necessary. Getting a crash is better than a silent change of behavior IMO.
  • Before merging, a couple of changes are required:
    • The docstring should be updated
    • The failing tests should be adapted to the new interface.
    • There should be new tests demonstrating that the new version works as intended (could be the examples you gave).

@mforets, @lucaferranti, what do you think?

@@ -19,7 +19,8 @@ function _branch_bound(bab::BranchAndBoundEnclosure, f::Function, X::Interval_or

fX = f(X) # TODO: allow user to choose how to evaluate this (mean value, natural enclosure)
# if tolerance or maximum number of iteration is met, return current enclosure
if diam(fX) <= bab.tol || cnt == bab.maxdepth
min_abs = in_interval(0, fX) ? zero(fX.lo) : min(abs(fX.lo), abs(fX.hi))
Copy link
Member

Choose a reason for hiding this comment

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

This is the definition of mignitude

Suggested change
min_abs = in_interval(0, fX) ? zero(fX.lo) : min(abs(fX.lo), abs(fX.hi))
min_abs = mig(fX)

Copy link
Member

Choose a reason for hiding this comment

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

Nice. Then I suggest to inline the function instead of having this extra line.

@lucaferranti
Copy link
Member

I dont think we need an alias, let's just rename tol to atol and make a breaking change. The new naming conention is also closer to that of other functions in the julia ecosystem

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