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

Compatibility with IntervalArithmetic v0.22 #203

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

Compatibility with IntervalArithmetic v0.22 #203

wants to merge 32 commits into from

Conversation

Kolaru
Copy link
Collaborator

@Kolaru Kolaru commented May 29, 2024

This PR aims at providing compatibility with IntervalArithmetic v0.22, which is breaking.

The user facing changes are

  • Decorated intervals must be used (which is the default Interval in the latest releases of IntervalArithmetic)
  • The signature of the roots function changed to roots(f::Function, X::Union{Interval, AbstractVector} ; kwargs...), with the following consequences
    • Manual derivatives and contractors must now always be passed as keyword arguments. This greatly simplify the internal logic of the roots function.
    • No more IntervalBox. Multidimensional problem are specified by returning a vector of intervals, and giving a vector of intervals as initial search region.
    • Normal vectors of intervals are supported. They are significantly (3x) time slower for a simple 2D problem than SVector, but more convenient.
  • Features of the packages outside of the roots function are unsupported for the time being (e.g. Slopes and quadratic equations). If you were using them, please open an issue.
  • roots works with @exact.

This PR does not yet contain an update of the documentation, but is otherwise ready for review (if the tests pass on CI ^^').

@Kolaru Kolaru closed this May 30, 2024
@Kolaru Kolaru reopened this May 30, 2024
@Kolaru Kolaru mentioned this pull request May 30, 2024
@OlivierHnt
Copy link
Member

OlivierHnt commented May 31, 2024

You are mentioning a slowdown, this is only due to using Vector instead of SVector?

Also, does this PR address some of the following issues #41, #52, #77, #98, #107, #117, #124, #157, #162, #181?

What is the status of #40 about complex intervals?

@Kolaru
Copy link
Collaborator Author

Kolaru commented May 31, 2024

I haven't adapted the benchmark yet, so I am not sure what is affected. Probably it everything is slightly affected, but mostly the few examples where I use Vector instead of SVector.

It fixes or superseeds #41 #52 #77 #107 #157
and also fixes without yet include tests #98 #162 #181 (Edit: I just added tests for those).

It half-fixes #40 as complex intervals are supported, but require explicit derivative.

It does nothing about #124 and propably make #117 a bit worse (roots.(f, Xs) should be used in that case).

@OlivierHnt
Copy link
Member

Out of curiosity, why is the keyword argument of roots called contractor? Why not alg for "algorithm"?

Also, a lot of examples in the docs you added have the NG flag, do you think it is worth using @exact to prevent them? But maybe that is more confusing.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Jun 10, 2024

Out of curiosity, why is the keyword argument of roots called contractor? Why not alg for "algorithm"

contractor is the most technically correct term, also according to the literature.

The algorithm is branch-and-prune, the contractor is only a small part of it. That being said, I am fine with any of contractor, algorithm, or method.

The doc should anyway be extended to discuss what's the difference between Newton and Krawczyk.

Also, a lot of examples in the docs you added have the NG flag, do you think it is worth using @exact to prevent them? But maybe that is more confusing.

I plan to discuss that separately. The _com_NG flag is purely related to IA.jl, and I think it is fair to assume users will either accept it, or know it from using IA in the first place.

To be fair, I have been considering not showing the decoration or flag of the interval in the roots. Instead, we could add a flag to the root object, when the assumption of the method are not fullfileld (i.e. the root interval has the NG flag, a decoration below dac, or even when the derivative doesn't have at least the dac decoration).

@OlivierHnt
Copy link
Member

To be fair, I have been considering not showing the decoration or flag of the interval in the roots. Instead, we could add a flag to the root object, when the assumption of the method are not fullfileld (i.e. the root interval has the NG flag, a decoration below dac, or even when the derivative doesn't have at least the dac decoration).

Do you think you can loose information by having a single global decoration and NG flag? For instance, in one of the example some roots have the "NG" flag and others don't.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Jun 10, 2024

In my idea, the information would be kept on the individual intervals, just not displayed. I assume that at a first glance, what's important is if the root is valid as a whole.

I'll keep that for another PR though, this one has already enough changes.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Jun 14, 2024

With the update to the doc, this PR is not fully ready for review.

@schillic schillic mentioned this pull request Jun 17, 2024
@OlivierHnt
Copy link
Member

Ok! I can help with the review once you decide your PR is ready.

@schillic
Copy link

With the update to the doc, this PR is not fully ready for review.

@Kolaru was the "not" a "now" with a typo? :)

@OlivierHnt
Copy link
Member

Yes I believe that was the case 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment