-
Notifications
You must be signed in to change notification settings - Fork 71
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
Conversation
Codecov ReportAttention:
❗ 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. |
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. 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 Also from PR #527, I kind of like the idea of displaying a NaI as |
There was a problem hiding this 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.
In triage, @lucaferranti pointed out that the flag for mixing I have added a new field to julia> sqrt(interval(-1, 1) + 0.5)
[0.0, 1.22475]_trv
julia> guarantee(sqrt(interval(-1, 1) + 0.5))
false |
How do we want to display an interval |
Perhaps, and only if Also, I would suggest to emit a warning or something like that. |
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. I will have to do some further changes to the code to throw a warning when we mix |
19443f6
to
7b2def0
Compare
I finished all files in Give me the weekend to go through the modified test files... |
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 |
There was a problem hiding this 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!
This PR renames
Interval
toBareInterval
, andDecoratedInterval
toInterval
. 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 theill
andtrv
decorations. This decoration occurs when a number is being converted to anInterval
(this behaviour is currently prohibited on master to prevent silent errors from generic functions):While we have
Interval <: Real
, the structureBareInterval
is not a subtype ofReal
. 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, butinterval(1) + 1
is allowed.Closes #219; closes #394; closes #419; closes #462; closes #568; closes #580; closes #584.
Other small changes
BareInterval
or anInterval
ifis_valid_interval
is false, this avoid flooding the IOinterval
constructor has a new keyword argumentformat
which is either:standard
(default) or:midpoint
. Now,±(m, r)
is a mere alias ofinterval(m, r; format = :midpoint)
and is defined in the submoduleIntervalArithmetic.Symbols
setformat
is renamed tosetdisplay
for clarity (since now we have the format keyword for interval which is not impacted by thissetformat
method)interval(1, Inf)
is displayed as[1, ∞)
instead of[1, ∞]
.@I_str
always return a decorated intervalTODO
EDIT:
Some functions are still missing to have complete parity betweenBareInterval
andInterval
, e.g. hyperbolic functions.Done.