-
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
1.0-dev: rework constructors #560
Conversation
Any comments/feedback on this PR? Since it involved altering many files, let me know if some things need to be clarified/modified 🙂 A "limitation" of this PR is that EDIT: Also, I am not so found of introducing the new macro |
I will have a deep look sometime this week. About |
and it will do the right thing (i.e. wrap the literals in correctly-rounded intervals). I think it's nice to keep that (and I don't think it can be replaced by a function), although I guess it's not really necessary. Probably |
Ah, I hadn't read @Kolaru 's comment properly. It's true that literals are parsed before macros; I guess we were doing some hacky workaround for that that should probably be removed! So maybe |
Yes the macro Here is a proposal: we remove |
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 only have small remarks for possible improvements. Nothing blocking it from being merge in my opinion (you are of course welcome to first remove @interval
as we discussed if you'd like).
Thanks a lot for the huge work, and especially for the efforts put in the docstrings !
|
||
""" | ||
atomic(::Type{<:Interval}, x) | ||
atomic(T<:Union{Rational,AbstractFloat}, a) |
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.
It can be documented as T <: NumTypes
.
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 thought about this but I did not know if we wanted to export NumTypes
or just keep it as an internal constant.
Whether we export NumTypes
or not, if we want to document it then I will add a docstring for NumTypes
.
Let me know :)
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.
Good point, I think then the wisest is to document this as T <: IntervalArithmetic.NumTypes
. It is not exported (NumTypes
should not I think), and it makes sure this docstring stay consistent if the definition of NumTypes
change).
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## 1.0-dev #560 +/- ##
===========================================
- Coverage 85.82% 84.79% -1.04%
===========================================
Files 34 33 -1
Lines 1841 1828 -13
===========================================
- Hits 1580 1550 -30
- Misses 261 278 +17
☔ View full report in Codecov by Sentry. |
Thank you @Kolaru for the review. |
Nightly is failing. It seems that this is due to the hashing mechanism; perhaps related to this recent commit JuliaLang/julia#49996? I do not know how this works, but perhaps testing against a specific hash value is not optimal? |
Your tests were fine. We just broke hashing on nightly. Should be fixed by the end of today (fix is merged, but CI sometimes takes a bit to see new Julia versions). |
@Kolaru is it possible for you to trigger the CI before merging? Just to double-check. |
Every light is now green, so I'merging. Thanks a lot ! This is really helping getting closer to the 1.0 dream :) |
This PR reworks the mechanism to construct an
Interval
.Minor changes:
default_bound
has been renameddefault_numtype
for consistency.atomic
to only accept the bound type; soatomic(Interval{T}, x)
is nowatomic(T, x)
.Standard constructors:
unsafe_interval(T, a, b)
is introduced as a replacement forInterval{T}(a, b)
which was unsafe, yet semantically very close to the safe constructorinterval
. The only remaining method forInterval{T}
isInterval{T}(::Interval)
which is safe since it relies oninterval
.checked_interval
function is now removed for clarity (it was already just a mere alias ofinterval
).±
can now be used to construct complex intervals when the midpoint is a complex number. EDIT: note that since±
is a midpoint-radius constructor, it is appropriate to defineradius(::Complex)
such that there is a consistency betweenx = m ± r
andmid(x) ± radius(x)
. This is defined in the complex.jl file but this file is not yetinclude
d into IntervalArithmetic.Macro constructors:
@floatinterval
and@biginterval
are now removed and replaced with the more general macro@tinterval
(where the letter "t" stands for "typed"). This new macro has the same capabilities as@interval
except that the bound type can be given as the first argument. In particular,@floatinterval expr
and@biginterval expr
become@tinterval Float64 expr
and@tinterval BigFloat expr
respectively.default_numtype()
is redefined to be aRational
, this is not used by default in@interval
and@I_str
. This feature comes in when calling@tinterval
, e.g.@tinterval Rational{Int} "0.1" + pi * 2//3
returns anInterval{Rational{Int}}
.These type of changes required doing quite a lot of editing in many files; I also took this opportunity to do some minor cleanup and improvements in docstrings.