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

Make decorated interval the default #590

Merged
merged 33 commits into from
Dec 1, 2023

Conversation

OlivierHnt
Copy link
Member

@OlivierHnt OlivierHnt commented Oct 30, 2023

This PR renames Interval to BareInterval, and DecoratedInterval to Interval. In effect, decorated intervals are now the default.

Goal

The main objective of this PR is the introduction of a new decoration, called bad (for badly-formed) which lies between the ill and trv decorations. This decoration occurs when a number is being converted to an Interval (this behaviour is currently prohibited on master to prevent silent errors from generic functions):

julia> using IntervalArithmetic

julia> convert(Interval{Float64}, 1)
[1.0, 1.0]_bad

julia> [1, interval(2)]
2-element Vector{Interval{Float64}}:
 [1.0, 1.0]_bad
 [2.0, 2.0]_com

While we have Interval <: Real, the structure BareInterval is not a subtype of Real. This makes using bare intervals much less flexible than their decorated counterparts, especially when it comes to inter-operability with the Julia ecosystem.
For instance, bareinterval(1) + 1 will throw an error, but interval(1) + 1 is allowed.

Closes #219; closes #394; closes #419; closes #462; closes #568; closes #580; closes #584.

Other small changes

  • no warning is emitted when trying to construct a BareInterval or an Interval if is_valid_interval is false, this avoid flooding the IO
  • the interval constructor has a new keyword argument format which is either :standard (default) or :midpoint. Now,±(m, r) is a mere alias of interval(m, r; format = :midpoint) and is defined in the submodule IntervalArithmetic.Symbols
  • setformat is renamed to setdisplay for clarity (since now we have the format keyword for interval which is not impacted by this setformat method)
  • display changed to show open interval if a bound is infinite; e.g. interval(1, Inf) is displayed as [1, ∞) instead of [1, ∞].
  • parsing macro @I_str always return a decorated interval

TODO

EDIT: Some functions are still missing to have complete parity between BareInterval and Interval, e.g. hyperbolic functions.
Done.

@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2023

Codecov Report

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

Comparison is base (c07cb3f) 85.62% compared to head (dfb71a6) 84.06%.

Files Patch % Lines
src/intervals/construction.jl 70.88% 46 Missing ⚠️
src/intervals/interval_operations/numeric.jl 74.83% 38 Missing ⚠️
src/intervals/arithmetic/hyperbolic.jl 67.85% 27 Missing ⚠️
src/display.jl 84.82% 22 Missing ⚠️
src/intervals/interval_operations/boolean.jl 86.45% 13 Missing ⚠️
...rc/intervals/interval_operations/set_operations.jl 45.45% 12 Missing ⚠️
src/intervals/real_interface.jl 76.92% 9 Missing ⚠️
src/intervals/arithmetic/trigonometric.jl 94.48% 8 Missing ⚠️
src/intervals/arithmetic/power.jl 96.17% 6 Missing ⚠️
src/intervals/arithmetic/basic.jl 95.40% 4 Missing ⚠️
... and 4 more

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
- Coverage   85.62%   84.06%   -1.57%     
==========================================
  Files          29       26       -3     
  Lines        1649     1970     +321     
==========================================
+ Hits         1412     1656     +244     
- Misses        237      314      +77     

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

@OlivierHnt
Copy link
Member Author

I found some interesting things discussed in the PR #527 from @lucaferranti.

According to the Standard, the bare interval part of a NaI (Not an Interval) is the empty interval, i.e. isempty_interval(bareinterval(interval(2, 1))) should be true. However, it currently returns false for floating-points bounds since both are NaN. On the other hand, since there is no NaN equivalent for rationals in Julia, isempty_interval(bareinterval(interval(2//1, 1//1))) == true as required by the Standard.

I think it may be good to fix this issue in this PR as well.

One approach would be to also address #408. That is, we internally represent an empty interval x with floating-point bounds as $[NaN, NaN]$ but keep the fact that inf(x) == Inf and sup(x) == -Inf. One benefit of doing this is that x.lo/x.hi will return NaN which will propagate throughout the computations.
For intervals with rational bounds, this means just keeping the current situation since there is no NaN equivalent in Julia.

Also from PR #527, I kind of like the idea of displaying a NaI as NaI, or NaI_ill, or ∅_ill; similarly to how we currently display the empty interval via .

@OlivierHnt
Copy link
Member Author

OlivierHnt commented Nov 1, 2023

Closes #408 and PR #527.
I also believe that this PR should close #237.

This was linked to issues Nov 1, 2023
Copy link
Collaborator

@Kolaru Kolaru left a comment

Choose a reason for hiding this comment

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

I started to look through the PR. That's a very impressive work, thanks again.

My strategy is to mostly trust the tests (that I haven't reviewed yet), since most of the changes are renaming or refactoring.

I'll try to continue review it part by part in the coming weeks.

src/display.jl Outdated Show resolved Hide resolved
src/intervals/arithmetic/basic.jl Outdated Show resolved Hide resolved
src/intervals/construction.jl Show resolved Hide resolved
src/intervals/construction.jl Outdated Show resolved Hide resolved
src/intervals/construction.jl Outdated Show resolved Hide resolved
src/intervals/construction.jl Outdated Show resolved Hide resolved
@OlivierHnt
Copy link
Member Author

OlivierHnt commented Nov 13, 2023

In triage, @lucaferranti pointed out that the flag for mixing Interval and Number can not really be a decoration. According to the Standard, the decoration is really there to monitor functions domain, while our decoration bad serves a completely different purpose.
For instance, ideally sqrt(interval(-1, 1) + 0.5) should be both bad and trv.

I have added a new field to Interval, called guarantee :: Bool (as suggested by @lucaferranti, but I have no personal preference on the name) which serves the purpose of the decoration bad. This field is always true, unless the interval was produced via conversion/promotion.

julia> sqrt(interval(-1, 1) + 0.5)
[0.0, 1.22475]_trv

julia> guarantee(sqrt(interval(-1, 1) + 0.5))
false

@OlivierHnt
Copy link
Member Author

OlivierHnt commented Nov 13, 2023

How do we want to display an interval x when guarantee(x) == false ?
There should be some sort of visual cue for the user.

@lbenet
Copy link
Member

lbenet commented Nov 13, 2023

How do we want to display an interval x when guarantee(x) == false ? There should be some sort of visual clue for the user.

Perhaps, and only if guarantee(x) == false, we could add some sort of second subscript (_bad or _notguaranteed) to make the point clear, e.g., [0,1]_trv_notguaranteed (or some abreviation like _NG).

Also, I would suggest to emit a warning or something like that.

@OlivierHnt
Copy link
Member Author

OlivierHnt commented Nov 14, 2023

That's a good idea. Now we have

julia> a = interval(1, 2)
[1.0, 2.0]_com

julia> a_NG = a / 1
[1.0, 2.0]_com_NG

julia> setdisplay(:full)
Display options:
  - format: full
  - decorations: true
  - significant digits: 6 (ignored)

julia> a_NG
Interval{Float64}(1.0, 2.0, com, NG)

Let me know if this is good enough.
Also, you cannot opt-out of showing "NG", which is probably a good thing.

I will have to do some further changes to the code to throw a warning when we mix Interval with Real.

src/symbols.jl Show resolved Hide resolved
@lbenet
Copy link
Member

lbenet commented Dec 1, 2023

I finished all files in src/. I left few more comments, as said, nothing essential, though for the Symbols submodule adding some hints on how to get the actual UTF symbol would be very helpful.

Give me the weekend to go through the modified test files...

@OlivierHnt
Copy link
Member Author

Sounds good, but do not waste time reviewing the ITF1788 test suite. Indeed, it will be overwritten in the next PR, since the test suite is now automatically generated (and the entire test suite is passing 🥳!).

So only the files in test/interval_tests are worth spending time on (but this should not be long).

Copy link
Member

@lbenet lbenet left a comment

Choose a reason for hiding this comment

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

Let me first thank you @OlivierHnt for all the enormous effort put in this PR. I agree with all your responses to my comments, perhaps just keeping in mind (i) operating with integers, and (ii) atomic. I am in favor of aproving this PR, and merge it! Thanks again!

@OlivierHnt OlivierHnt merged commit 742a26f into JuliaIntervals:master Dec 1, 2023
16 checks passed
@OlivierHnt OlivierHnt deleted the decoration branch December 1, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment