[WIP / RFC] Configuration mechanism for directed rounding and fast powers #388
Conversation
dpsanders
changed the title
dpsanders
changed the title
|
Note that the mechanism itself is still the same: globally redefine all functions according to the selected mode. There does not seem to be a workable alternative to this. |
|
I think this is conceptually fine. One way of another, changing the behavior of function must somehow be a redefinition anyway. For our own testing purpose, I think it would be good to have a do-block syntax available, as it will likely make tests more readable and safer. |
|
@Kolaru Thanks for the feedback. I agree that having a do-block syntax would be good for testing purposes. I'm not sure I'll manage that in this PR though. |
|
Bump 😄 Is this "only" needing to add testing and docs? |
|
And probably other rounding modes, yes. |
|
What is the current state of this PR? |
|
@lbenet I have fixed the merge conflicts so this should be ready for review / merge. Couple of points
|
however looking at the rounding.jl file it seems to be still there and the docstring is just outdated |
|
fixes #14 |
There was a problem hiding this comment.
Thanks a lot @lucaferranti for dealing with rebasing to current master.
In general, LGT. Still, Ileft few comments, the important ones related to testing this (instead of commenting the tests), and also about changing the defaults. I think the actual only requirement is to bump to the next minor version.
src/intervals/functions.jl
Outdated
| function cbrt(a::Interval{T}) where T | ||
| isempty(a) && return a | ||
|
|
||
| @round(cbrt(a.lo), cbrt(a.hi)) | ||
| end |
There was a problem hiding this comment.
cbrt is obtained through CRlibm, right? That's the reason to delete it here, isn't it?
src/intervals/intervals.jl
Outdated
| a = _normalisezero(a) | ||
| b = _normalisezero(b) | ||
|
|
There was a problem hiding this comment.
Is this file changing with respect to master? It shouldn't, right?
| @eval ^(::PowerType{:tight}, a::Interval{Float64}, x::$T) = atomic(Interval{Float64}, bigequiv(a)^x) | ||
| @eval ^(::PowerType{:tight}, a::Interval{Float32}, x::$T) = atomic(Interval{Float64}, bigequiv(a)^x) |
There was a problem hiding this comment.
Should we impose that a::Interval{T} and x::T?
| # name mismatch | ||
| if directed_rounding == :fast | ||
| directed_rounding = :accurate | ||
| end |
There was a problem hiding this comment.
We should only use one of them, say :accurate...
| const allowed_values = Dict(:directed_rounding => (:tight, :fast, :none), | ||
| :powers => (:tight, :fast)) | ||
|
|
||
| function configure!(; directed_rounding=nothing, powers=nothing) |
There was a problem hiding this comment.
What about fixing the defaults here, instead of nothings ...?
| @@ -80,7 +81,7 @@ export | |||
|
|
|||
| export showfull | |||
|
|
|||
| import Base: rounding, setrounding, setprecision | |||
| import Base: setprecision | |||
There was a problem hiding this comment.
Since we are getting rid of rounding and setrounding, shouldn't we amend (in parallel) the docs? Similarly, we should add the corresponding docs of IntervalArithmetic.set_directed_rounding
| const configuration = Dict(:directed_rounding => :tight, | ||
| :powers => :fast) |
There was a problem hiding this comment.
I think we should have as default configuration that both defaults are :tight; It seems to me a bit unnatural to have (:directed_rounding => :tight and :powers => :fast. Note that tests change to this mode.
| end | ||
|
|
||
| """ | ||
| setrounding(Interval, rounding_type::Symbol) | ||
| set_directed_rounding(rounding_type::Symbol) |
There was a problem hiding this comment.
This is a simple reminder to add the proper docs to https://juliaintervals.github.io/pages/tutorials/tutorialArithmetic/index.html#advanced_options_precision_and_rounding_modes
| @@ -10,6 +10,6 @@ include("linear_algebra.jl") | |||
| include("loops.jl") | |||
| include("parsing.jl") | |||
| # include("rounding_macros.jl") | |||
| include("rounding.jl") | |||
| #include("rounding.jl") | |||
There was a problem hiding this comment.
Shouldn't we adapt the tests to the changes implemented here?
|
Superseded by PR #593. Let us re-visit this PR if the power mechanism fails to address all the concerns. |
This is a proposal for a better configuration mechanism for changing the directed rounding method (tight or accurate) and for choosing the type of powers (fast or tight).
Usage:
This would fix several outstanding issues (that I won't list right now).
This is the minimal thing that makes sense to me. The syntax is nasty enough that it feels like something a casual user should not do (unlike the previous
setrounding(Interval, :tight)); in particular I am proposing not to exportconfigure!), but light enough that it is not unusable. In any case this is designed to be used a very minimal number of times in any given piece of code.Tests and docs need updating but I would appreciate feedback.
cc-ing interested parties: @lbenet, @Kolaru, @gwater, @JeffBezanson, @mforets
fixes #103