-
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
Implement systems for flavor, pointwise extensions, default bound and rounding mode #271
Conversation
While reviewing the files
Is there any known performance gain to the former? I play a bit with it and wasn't able to see any difference (I assume the compiler may be smart enough to give correct type to literals). Unifying that will either have a good impact on performance or on readability, so I thing it's worth looking into. @dpsanders @lbenet I assume one of you wrote this files in the first place, so any idea? |
Please, ping us when this is ready for review... |
This looks really fantastic, thanks! Will it mess you up too much if we merge a few of the latest prs? |
As far as the comparisons with zero go, I think they are equivalent, but yes we should be consistent. I think comparing with the literal zero is sufficient. |
Of course this PR is not yet ready for review (forgot to comment after pushing), I pushed these changes to show my current progress (they include a Also most basic functionality should work (constructing and arithmetic with intervals), yet many tests fail or error. Reasons for this include the new system 100% breaking the old promotion mechanism and some crucial functions for test changing names, like @dpsanders I think you should not hesitate, most of them have a well defined scope and I should be able to adapt this PR to them without too much fuss (hopefully). Moreover I can't give an estimation of the time it will take to finish this PR, promotion is tricky and the test suite to which I should comply (for each provided flavor) is huge. |
I have encountered a problem deeper that what I had anticipated: julia> @which 2*(1..2)
*(x::Number, y::Number) in Base at promotion.jl:314 This means that if we allow flavor that are not subtype of at least While not immediatly blocking for the developpement of the this PR, I think it's worth discussing as removing |
Can you elaborate? To me it seems consistent, that something which is not a |
You don't need to reimplememt promotion, just provide methods like +(x::Interval, y::Real), which is not too onerous. |
I'll answer here to @dpsanders about the comment I made in #335 about "being overdramatic about a minor setback": The setback to which I referred is the promotion mechanism (or lack thereof for But at the end of the day, it is more a philosophical matter, as the code base won't looks very different either way (and may even looks cleaner not using the |
These days I would definitely say to not use the promotion mechanism. Just define all the functions explicitly. |
Note that getting rid of the promotion mechanism also means removing conversion between number and interval with the I'm perfectly fine with that, it simply forces the conversion to go through an interval constructor. It is worth noting however since some of the oldest issues mention the implementation of new |
Is it possible that you rebase this to current master? |
nervous laughter Yeah sure, ha ha, no problem... Truth is, since I pretty much change every file in this PR and reorganize many of them, rebasing will mean erasing everything and reintegrating manually all changes in master on top of it, which is a rather consequent work. Is there a reason you wish to have a rebased version before the PR is ready for review/merging ? |
Just thinking about this comment:
and how should we all proceed in a constructive way... |
In many cases new PR would need to be adapted to the new architecture (including a new file organization to match the IEEE standard) developped there. While rebasing will be serious work, it will mostly be a matter putting the changes in the right file together with some renaming. So considering that this PR still needs a lot of work, I think it makes more sense to not block other PRs and deal with the rebase problem at the end. I'll make sure to ask for review and merging as soon as the full current test suite pass for a single flavor though, to allow to constructively build future change on top of this PR (including among other new flavors). |
This what I had precisely in mind... |
I'm wondering if some of the pieces can be gradually introduced, such as the changes to the abstract base types, without much disruption. Or maybe it would be better to develop a separate package like |
I'm in favor of the first option. |
What I'm doing here is:
It doesn't make sense to split the process in between these steps, but what I can do to make this incremental is to make smaller PR for each chunk of the codebase I review, the rest of the package should not be broken as everything will still be defined for the default First chunk would be everything I've done until now (most basics functionnalities), because my commit history has been to messy to have nice smaller chunks ^^'. I still need to fix all the failing tests I currently have and rebase it and we would be good to go, if you think that's the way to do it. |
That sounds great. We should also set up some benchmarks to make sure there are no performance regressions. |
@lbenet I've gone through you review once. Thanks very much for the depth of it. I modified the simple things. What is still open is either to be discussed or, if I haven't commented on it, is is that I will need a bit more time to think about it with. |
Thanks @Kolaru for solving the trivial stuff; I think we can slowly proceed to the deeper stuff, some of which requires some discussion. I'm playing with this PR and the ieee tests. As I mentioned elsewhere, we should define a |
# NOTE this interacts with flavors. | ||
function isinf(::PointwisePolitic{:ternary}, x::Interval) | ||
if contains_infinity(x) | ||
isthing(x) && return true |
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.
isthing
-> isthin
has_false::Bool | ||
|
||
function BooleanInterval(a::Bool, b::Bool) | ||
a == b && throw(ArgumentError("boolean interval with content [$a, $b] doesn't make sense.")) |
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.
a == b && throw(ArgumentError("boolean interval with content [$a, $b] doesn't make sense.")) | |
a == b && throw(ArgumentError("boolean interval [$a, $b] is not allowed.")) |
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.
actualy, I don't think this line is needed, I think BooleanInterval(true, true) and BooleanInterval(false, false) should be allowed
Appreciations
Important practical comments
Comments
Flavor{:set} -- the IEEE standard
Flavor{:number} -- the one using ternary logic
Flavor{:etc} -- another flavor then we could use the abstract type
AbstractInterval <: Real # current situation, not saying I agree :)
Interval <: AbstractInterval
DecoratedInterval <: AbstractInterval
|
For a constructor that complies with the requirement of a constructor | ||
according to the IEEE Std 1788-2015, use one of the other convenience | ||
constructors: | ||
- `checked_interval(a, b)` Explicitly checked interval. |
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.
why not using interval
instead of checked_interval
as it currently is? It would be less breaking and less typing. Also, I don't think we need both checked_interval
and ..
if they behave the same.
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 always found interval
vs Interval
rather confusing, but don't have a particularly strong opinion here either. Especially if we keep ..
and Interval
to nearly synonymous (in the sense we don't do anything smart to widen the bounds).
Then again, the advantage if we keep interval
only is that I can replace all occurrence in one go with Ctrl + Maj + F
:).
|
||
interval(a::Real) = interval(a, a) | ||
|
||
const checked_interval = interval |
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 think if is_valid_interval
is made flavor-dependent then that would be enough?
isthinzero(b) && return c | ||
return entireinterval(F) | ||
elseif isentire(b) | ||
isthinzero(a) && return c |
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.
fwiw, I had updated fma not to use setrounding in another PR, but I can rebase it later if this is merged first
has_false::Bool | ||
|
||
function BooleanInterval(a::Bool, b::Bool) | ||
a == b && throw(ArgumentError("boolean interval with content [$a, $b] doesn't make sense.")) |
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.
actualy, I don't think this line is needed, I think BooleanInterval(true, true) and BooleanInterval(false, false) should be allowed
|
||
function BooleanInterval(a::Bool, b::Bool) | ||
a == b && throw(ArgumentError("boolean interval with content [$a, $b] doesn't make sense.")) | ||
return new(true, true) |
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.
this should be new(true, false)
?
@@ -0,0 +1,233 @@ | |||
Review intervals folder: |
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.
could you please gitignore this and the changelog? It adds a lot of noise when browsing the PR from github.
@test A \ b == [-5..5 | ||
-4..4] | ||
|
||
# TODO This is using the wrong \. I think the correct one is defined |
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 think this should be removed (also it would error if the ternary logic defined here is used).
A few curiosities if you are interested: The \
defined in IntervalRootFinding preconditions by the approximate inverse midpoint. Preconditioning an interval linear system actually enlarges the solution set (fun fact, the solution set of this is the logo of IntervalLinearAlgebra.jl), so the backslash overloaded there would return [-14..14, -14..14], which is the interval hull of the preconditioned solution set.
@test sin(@biginterval(0.5, 8.5)) ⊆ sin(@interval(0.5, 8.5)) | ||
@test sin(@biginterval(-4.5, 0.1)) ⊆ sin(@interval(-4.5, 0.1)) | ||
@test sin(@biginterval(1.3, 6.3)) ⊆ sin(@interval(1.3, 6.3)) | ||
@test sin(Interval(0.5)) ≛ Interval(0.47942553860420295, 0.47942553860420301) |
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 think tests should have interval
or ..
(the one that doesn't do black magic) on the LHS and Interval
on the RHS. The reason for this is that you want to use on the left a constructor that does the checks that has to be done (e.g. check that errors when an invalid input is given), but at the same time at the RHS you want the ground truth as given without any processingn, otherwise two errors on both sides could cancel out each others. (ITF1788.jl generates the tests with this logic)
Finally, I think in testing === should be preferred.
|
||
# ITF1788 tests | ||
|
||
include("ITF1788_tests/ITF1788_tests.jl") | ||
# include_test("ITF1788_tests/ITF1788_tests.jl") # TODO fix these tests |
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.
if/when they are re-included, I think it would make sense to have all of them clustered in one folder as they were before. (I am not sure now if they have been just temporarily removed or just scattered around)
@test @decorated(-3, 2)^@decorated(0.0, 1.0) == DecoratedInterval(0.0,2.0, trv) | ||
@test @decorated(-3, 2)^Interval(-1.0, 1.0) == DecoratedInterval(0.0,Inf, trv) | ||
@test @decorated(-3, 2)^@decorated(-1.0, 1.0) == DecoratedInterval(0.0, Inf, trv) | ||
@test @decorated(2,3) ^ 2 ≛ DecoratedInterval(4, 9) |
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.
the ≛ is the standard compliant right? If it is, then it doesn't check that the decoration matches, should use either === or check for decoration separately (again, ITF1788.jl takes care of this, so it hopefully will help minimize manual writing)
I've been playing with this PR over the weekend (trying to make ITF1788, and I've came a bit far making lots of things work!) and believe that I have now a clearer view on how the new scheme works. Lots of things are already described in Benoît's comment, but it was until I tried to get my hands dirty that they became clearer. First, I agree with Luca's comment that we should split somewhat the reviewing/testing/developing work on this PR. Rather than merging this PR in a new dev-branch, I propose that Luca, David and me work (in parallel) in a new PR (branching from this one), so Benoît can cherry-pick the commits that he judges help; I think this should be enough but I may be wrong. (I will open a new PR with some suggested changes this afternoon.) For the same reason, we need very concrete commits. I also agree with Luca's comment that the Regarding the design, which I like very much, there is something that is currently uncomfortable: If I need to change for example the |
the reason for proposing a dev-branch here is that this PR is opened from kolaru's fork, hence if we open PRs to this branch (which I think is what @lbenet proposed?), they go the other repository, I think for reviewing and developing it's easier to have everything in this repository. Ideally, I think this PR should be broken into smaller ones. I think the main issue is that a lot of independent things are done here (rewrite the parser, restructure the folder, change constructors, introduce pointwise policy, introduce flavor system). This makes review and work very hard. Ideally a PR should do only one self-contained change, this is also what is suggested by colprac, the SciML contributor guideline. The fact that the PR has both restructured the folder and changed code makes reviewing particularly hard, since github cannot distinguish between what is new and what has just been moved. One option would be to keep this as a template and rebuild it in parallel via smaller PRs to master branch, this would make discussion and review easier and also introduce the changes gradually, which makes adoption by dependent packages smoother. However, I understand this can feel unfair to @Kolaru, who has put an enormous amount of work on this, hence I guess it's ok to keep pushing to this PR (or a clone in the repository). For the pointwise policy, yes I also think now it makes sense to have it as subflavour, because also rounding mode is changed independently. This also reduces the number of states. Inspired by @dpsanders PR about configuration mechanism: I think one thing we could do is to have a function function config(;
rounding_mode=:tight,
flavor=:set,
comparison=:ieee1788
)
@eval ==(a::Interval, b::Interval) = ==(PointwisePolicy{comparison}, a, b)
.... the idea is to have each function defined as |
My proposal is more basic, I think: let's open a parallel PR from the branch corresponding to this PR (so, including the last commit uploaded by @Kolaru). That branch lives here, so we may upload commits there. And @Kolaru may cherry-pick the commits that solve punctual issues that we comment here.
I agree. So this is what I've done: check the way that each file is Finally, I like the idea of the |
@lucaferranti Thanks a lot for the comments. It looks like I am already lagging behind a bit. I will try to catch up. If I somehow miss something and forget to go back to it, please let me know. Here are my comments on the general review, I'll address the comments later.
As far as I understand it is what the package is currently doing. I am a bit surprise that it doesn't comply with the standard, since the standard does not define those operations, as far as I can remember. Are we sure we all speaking of the same thing?
First, I am very open to name suggestions :D. Then for the other points, here was my reasoning:
Sometimes I let myself be optimistic :D. The promise I thought I was making are indeed way lower that what you describe. What I meant is the following: With this flavor the worst cases are:
While with the current behavior, the worst case is: your results are wrong. So the
I should have been more explicit about that: I also think that
Realistically, for now we probably need 2 flavors. The one required by the standard, and another that handles infinity in a more "natural" way, because some users have been confused by the things like Concerning pointwise politic, we probably only need one. I added multiple because it was easy enough and showed how to manage them.
I have no strong opinion on whether to use the type system or a global state for that. The arguments were essentially as follow:
If we go for the former, we will have to decide if we want the flavor to be a type parameter i.e. |
Normally just typing |
I may have been slightly overboard all at once, it is true. However, while it was not the initial intent, I think this PR is mostly restructuring, trying to clarify what is standard compliant and fixing/simplifying what is straightforward ("what is straightforward" did undergo strong inflation during the course of writing this PR, leading to the unfortunate current state of affair). What I can do without too much trouble (hopefully) is stripping this PR from the parts that requires discussion and go beyond restructructuration:
|
We probably already have enough to discuss in the upcoming days, but just putting there that this PR introduces some problems with matrices of intervals, probably due to the removal of the promotion mechanism
2×2 Matrix{Interval{Float64}}:
[1, 1] [1, 1]
[2, 2] [3, 3]
julia> [1 1..1;2 3]
2×2 Matrix{Real}:
1 [1, 1]
2 3 I think this would be problematic for many dependents (e.g., I think in IntervalLinearAlgebra it would cause some problems and would make the package harder to use). Why was the idea behind removing the promotion mechanism? |
It caused hard to understand bug. Generally either And it was a bit complicated. Having gone through everything, I think we could simplify it to essentially just promote(::Type{Interval}, x::Real) = Interval(x) |
yes I also think to remember the promotion/conversion code seemed a little overcomplicated. However, I think we need the following at least convert(Interval{T<:Real}, x::Real) # I think this is used internally, e.g. in the matrix example above
convert(Interval, x::Real)
convert(Interva{T<:Real}, x::Interval{S<:Real}) # not sure if this is needed but can be useful and probably a few for complex intervals, but let's discuss those later maybe |
Just to have it in mind for an eventual implementation of another flavor: Non-traditional intervals and their use. Which ones really make sense? by Sergey P. Shary. |
Some algorithms to bound the solution set of parametric interval linear systems also use Kaucher intervals (although they just use dual of intervals and nothing fancier) |
Update 1.0-dev with some commits from master (not in #271)
Historically interested, and now superseeded by other developements. |
This PR is my latest take on solving #237 and is a replacement #254. The main reason for a new PR and the closing of the previous one is that #254 relied heavily on macro and was thus hard to maintain and even harder to extend. Moreover the discussion around #254 arrived at the conclusion that what we want is really what the standard call a "flavor", and thus trying to come up with a flavor system seems the way to go.
In summary my first try in #254 was both misguided and overcomplicated.
The new trick is simpler and is essentially contained in the following lines:
Concrete interval types must subtype either
AbstractRealFlavor
orAbstractNonRealFlavor
and will benefit from all methods defined forAbstractFlavor
. Thus this system should be extensible and relatively easy to maintain as for the most part we can treatAbstractFlavor
as the common supertype of all intervals (it is however not as it can not be directly subtyped).Currently this is still a work in progress, but the package compile, thanks to the definition of a default interval type named
Interval
. Thus most old definitions work but are only defined for the default flavor (note that some functionnality like the..
syntax will always only be defined for the default interval type).What remain to be done is
I do not forget the problem of boolean functions that were at the core of #254 and solutions to these problems to them will be implemented in the corresponding flavors.
PS: Since this requires to review more or less the whole codebase I'm also trying to clean up the code along the way.
List of issues that should be fixed by this PR or that this PR will help solve:
isinf()
will be flavor dependant.Real
.[∞, ∞]
as a valid interval.mod
may be flavor dependant.Issues I will probably fix along the way as they are minor and I'll need to review related part of the code, but that are tangential to the core concerns:
entireinterval
is not a very good name #8