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

Beta 0.22 #593

Merged
merged 54 commits into from
Dec 13, 2023
Merged

Beta 0.22 #593

merged 54 commits into from
Dec 13, 2023

Conversation

OlivierHnt
Copy link
Member

@OlivierHnt OlivierHnt commented Dec 1, 2023

With all the progress made recently, we should be a few commits away from releasing 0.22! If everything goes well, this should be the last version before the 1.0 release 🥳!

Changes

To do:

To do list (perhaps not necessary for 0.22):

Some thoughts on naming convention

I would like to take this opportunity to review once more the name changes that have occurred since 0.20. One big change is that we decided to not overload Base boolean functions (e.g. ==, issubset, etc.) to prevent silent errors. We have appended the suffix _interval to distinguish the functions (e.g. issubset_interval vs issubset from Base).

Tim Holy's naming convention in ThickNumbers.jl, is to use the suffix tn as a short form of "thick number". Perhaps we want to use something similar to have shorter function names, e.g. the suffix _itl (short for "interval") to replace _interval.

Post-scriptum

Should bisect at any other point than 0.5 work for unbounded intervals?

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2023

Codecov Report

Attention: 196 lines in your changes are missing coverage. Please review.

Comparison is base (742a26f) 84.06% compared to head (10c348e) 83.01%.

Files Patch % Lines
src/intervals/construction.jl 66.98% 35 Missing ⚠️
src/intervals/arithmetic/power.jl 84.40% 34 Missing ⚠️
src/intervals/interval_operations/boolean.jl 53.33% 28 Missing ⚠️
src/intervals/arithmetic/hyperbolic.jl 67.64% 22 Missing ⚠️
src/intervals/interval_operations/constants.jl 29.16% 17 Missing ⚠️
src/intervals/interval_operations/bisect.jl 73.21% 15 Missing ⚠️
src/intervals/arithmetic/trigonometric.jl 92.30% 10 Missing ⚠️
src/intervals/rounding.jl 91.58% 9 Missing ⚠️
...rc/intervals/interval_operations/set_operations.jl 90.27% 7 Missing ⚠️
src/intervals/interval_operations/numeric.jl 77.27% 5 Missing ⚠️
... and 6 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
- Coverage   84.06%   83.01%   -1.05%     
==========================================
  Files          26       23       -3     
  Lines        1970     2072     +102     
==========================================
+ Hits         1656     1720      +64     
- Misses        314      352      +38     

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

@OlivierHnt
Copy link
Member Author

OlivierHnt commented Dec 2, 2023

It seems that there is still more work to be done to have better ForwardDiff.jl compatibility. I have added a few broken tests to keep track of this a little.

For instance, ForwardDiff.derivative(x -> x^interval(2), interval(-1, 1)) throws a MethodError due to method ambiguity; which is probably not so bad to fix.

On the other hand, ForwardDiff seems to rely on T(::Real), where T<:Real, for conversion. This is something we have discontinued in favour of explicit convert call; but perhaps we can re-allow it (with the extra isguarranteed == false of course). Perhaps simply

Interval{T}(x) where {T<:NumTypes} = convert(Interval{T}, x)

@lbenet
Copy link
Member

lbenet commented Dec 3, 2023

Looks again like a giant PR is coming 😄 (or 🥲 ? )... Thanks again for the enourmous effort.

Few comments (note that apriori I do not opose to the changes):

  • changed behaviour of functions in numeric.jl for numbers; e.g. sup(Inf) = NaN (Inf is first converted into an Interval), previous behaviour sup(Inf) == Inf

    Can you explain the motivation? It seems that this is some default method, to be appied to Reals. Note in pass that sup(5.0) returns 5.0 (not an interval), so I think returning Inf is consistent...

  • change default bisection to be at 0.5 by default

    If I recall correctly, the value (127//256) was considered to "break slightly the symmetry" (for symmetric intervals) which in somecases complicates applying IntervalRootFinding methods; in other words, breaking such symmetry permitted faster convergence. (I think this was inspired by a comment in the book by Tucker.)

  • remove atomic since it is an unused internal constructor (see also Make decorated interval the default #590 (comment))

    We could also consider keeping atomic if it indeed returns an atomic interval (either a thin interval of a one-eps wide interval, except perhaps for unbounded intervals). I'm thinking about the possibility to use it to construct tight intervals, that do contain the actual real number, as atomic(Float64, "0.1") does. Issues about performance can be stated in the docs. This is, at the end, also related to rounding.

  • A side comment, related to the ITF1788 test suite: I am in favor of having the rev functions within IntervalArithmetic; I truly do not see any scenario of using such functions separated from IA...

  • I don't understand the to-do:

    rework rounding to stop doing type piracy (currently we overload sqrt, *, /, etc from Base)

    My point is, rather than type-piracy, we do method-overloading exploiting multiple dispatch. I am in favor of keeping those functions.

  • Perhaps the to-do and to-do list should be addressed in a separate PR... makes life easier to review. The question is if the compatibility with ForwardDiff should be also addressed in another PR. Once said this, if T(x) is needed by ForwardDiff, your solution seems ok with me. The question is if we can use ForwardDiff in a guaranteed form.

LICENSE.md Outdated Show resolved Hide resolved
Project.toml Show resolved Hide resolved
docs/src/manual/usage.md Outdated Show resolved Hide resolved
@OlivierHnt
Copy link
Member Author

OlivierHnt commented Dec 13, 2023

Mhm @interval has very poor performance; this is due to parse, e.g.:

julia> @time IntervalArithmetic.atomic(Float64, 1)
  0.000029 seconds (14 allocations: 644 bytes)
[1.0, 1.0]_com

EDIT: with the last commit, this is down to 8 allocations (300 bytes); there is still room for improvement.

@lbenet
Copy link
Member

lbenet commented Dec 13, 2023

Just noticed the following (which relies on Base methods!):

julia> a = interval(-1, 1)
Interval{Float64}(-1.0, 1.0, com)

julia> b = complex(a, a)
Interval{Float64}(-1.0, 1.0, com) + Interval{Float64}(-1.0, 1.0, com)im

julia> zero(a)  # OK
Interval{Float64}(0.0, 0.0, com)

julia> zero(b)  # Result is ok, except perhaps for the NG (due to using a method from Base)
Interval{Float64}(0.0, 0.0, com, NG) + Interval{Float64}(0.0, 0.0, com, NG)im

julia> @which zero(b)
zero(x::Number)
     @ Base number.jl:308

Maybe we should open a new issue with the most basic operations we want to have for ComplexI... (Truly, I adore that alias!)

@OlivierHnt
Copy link
Member Author

I added a specialised method for zero and one.

We should perhaps merge this PR and release 0.22? This will allow us getting more feedback from downstream packages during December.

@lbenet
Copy link
Member

lbenet commented Dec 13, 2023

Please go ahead! I fully agree merging this PR and releasing v0.22 will allow us to check everything more reliably. Great work!

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