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

Implement systems for flavor, pointwise extensions, default bound and rounding mode #271

Closed
wants to merge 94 commits into from

Conversation

Kolaru
Copy link
Collaborator

@Kolaru Kolaru commented Mar 26, 2019

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:

abstract type AbstractRealFlavor{T} <: Real end
abstract type AbstractNonRealFlavor{T} end

const AbstractFlavor{T} = Union{AbstractRealFlavor{T}, AbstractNonRealFlavor{T}}

Concrete interval types must subtype either AbstractRealFlavor or AbstractNonRealFlavor and will benefit from all methods defined for AbstractFlavor. Thus this system should be extensible and relatively easy to maintain as for the most part we can treat AbstractFlavor 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

  • Separate the functionnalities common for all flavors from the flavor dependant ones and redefined them accordingly.
  • Implement all required functions for the IEEE standard "set based flavor".
  • For each function in the IEEE standard, add ref. to where it is defined in the IEEE standard.
  • Add all relevant flavors.
  • Fix all the bugs.

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:

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:

@Kolaru
Copy link
Collaborator Author

Kolaru commented Aug 17, 2019

While reviewing the files arithmetic.jl and functions.jl I noticed an inconsistent way of comparing numbers to zero:

  1. In arithmetic.jl by first getting the zero of the right type, typically x >= zero(x).
  2. In functions.jl by comparing to literal, without any conversion, typically x >= 0.

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?

@Kolaru Kolaru mentioned this pull request Aug 19, 2019
@lbenet
Copy link
Member

lbenet commented Aug 23, 2019

Please, ping us when this is ready for review...

@dpsanders
Copy link
Member

This looks really fantastic, thanks!

Will it mess you up too much if we merge a few of the latest prs?

@dpsanders
Copy link
Member

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.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Aug 23, 2019

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 TODO and changelog.txt files if you want to have a quick look at the state of affair).

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 == becoming isidentical for intervals.

@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.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Aug 25, 2019

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 Number, they won't be able to use the built-in promotion mechanism. Unless we are willing to reimplement a promotion ourselves (I don't volunteer for that), I think it forces us to have Flavor <: Number for any flavor (and probably Flavor <: Real would make sense since interval are as much number are they are real... that is they are not).

While not immediatly blocking for the developpement of the this PR, I think it's worth discussing as removing <: Real was the main issue tackled by this PR.

@gwater
Copy link
Contributor

gwater commented Aug 26, 2019

This means that if we allow flavor that are not subtype of at least Number, they won't be able to use the built-in promotion mechanism.

Can you elaborate? To me it seems consistent, that something which is not a Number should not go through the normal promotion rules. How non-Number objects interact among themselves and with numbers naturally depends on the underlying mathematical structure of those objects.

@dpsanders
Copy link
Member

You don't need to reimplememt promotion, just provide methods like +(x::Interval, y::Real), which is not too onerous.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Dec 4, 2019

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 Intervals that are not Numbers) I mentioned in my previous comment here. While I now agree that it would not be too hard to play around it, it still feels a bit like reinventing the wheel. In some sense being a Number mainly means accessing to that mechanism (the problematic comparisons operators for example aren't defined at that level).

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 Number promotion mechanism since the promotion rule stuff is kind of a mess).

@dpsanders
Copy link
Member

These days I would definitely say to not use the promotion mechanism. Just define all the functions explicitly.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Dec 11, 2019

Note that getting rid of the promotion mechanism also means removing conversion between number and interval with the convert function (since it is automatically called for arithmetic operations otherwise and it messes up everything).

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 convert methods.

@lbenet
Copy link
Member

lbenet commented Dec 11, 2019

Is it possible that you rebase this to current master?

@Kolaru
Copy link
Collaborator Author

Kolaru commented Dec 11, 2019

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 ?

@lbenet
Copy link
Member

lbenet commented Dec 11, 2019

Just thinking about this comment:

Will it mess you up too much if we merge a few of the latest prs?

and how should we all proceed in a constructive way...

@Kolaru
Copy link
Collaborator Author

Kolaru commented Dec 11, 2019

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).

@lbenet
Copy link
Member

lbenet commented Dec 12, 2019

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.

This what I had precisely in mind...

@dpsanders
Copy link
Member

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 IntervalArithmetic2, which when it is ready can just completely replace the current package.

@lbenet
Copy link
Member

lbenet commented Dec 12, 2019

I'm in favor of the first option.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Dec 12, 2019

What I'm doing here is:

  1. Implementing a new type hierarchy.
  2. Extend the type hierarchy to support new flavors.
  3. Review each file to put the definition at the right level (AbstractFlavor, specific flavor or default Interval).
    3a. Check what the standard require for 3.
    3b. Since I review each file anyway, restructure the file system and add docstring according to the standard.
  4. Fix all tests that gets broken along the way.

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 Interval flavor.

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.

@dpsanders
Copy link
Member

That sounds great.

We should also set up some benchmarks to make sure there are no performance regressions.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Jan 22, 2022

@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.

@lbenet
Copy link
Member

lbenet commented Jan 22, 2022

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 PointwisePolitic which is consistent with those tests. I hope to come back with a more definite proposal here.

# NOTE this interacts with flavors.
function isinf(::PointwisePolitic{:ternary}, x::Interval)
if contains_infinity(x)
isthing(x) && return true
Copy link
Member

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."))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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."))

Copy link
Member

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

@lucaferranti
Copy link
Member

lucaferranti commented Jan 22, 2022

Appreciations

  • This is a very impressive job!
  • The new structure of the folder seems much clearer
  • You rebased the whole thing!!! WoW, that's just... WoW! Amazing!

Important practical comments

  • This PR is already very big. I suggest to merge this asap into a flavor-dev branch and add smaller atomic PRs to that, the advantages of this are

    • easy review process
    • Kolaru doesn't have to do all the work while we just sit back and complain / ask for changes :)
  • I have never used github wiki, but aren't they meant for discussions? Maybe it's better to have discussions there instead of here? Experiences with that?

  • I think a call to discuss design principles and tasks division could be good, but if you prefer asynchronous communication here, it's fine for me too.

Comments

  • Pointwise policy general: In the current implementation the pointwise policy seems to be orthogonal to the flavor system, that is one needs to choose the flavor and the pointwise policy. I am not fully convinced by this, I think each pointwise policy should be one flavor. That is the flavor system could look something like this:
Flavor{:set} -- the IEEE standard
Flavor{:number} -- the one using ternary logic
Flavor{:etc} -- another flavor

then we could use the abstract type Flavor for functions shared by all flavors and union-types for functions shared by some but not all flavors.

  • all-in policy: Your docstring sounds a little judgy towards this policy and I agree with you. It inherits all downside of interval comparisons and is not compliant to the standard, so do we even need this? I think YAGNI together with KISS are the design patterns we should follow in this PR, which is already big.

  • interval: I think this should be renamed to interval-boolean or something. As someone who also works with fuzzy logic, I like this, a few suggestions:

    • I don't see any damage in allowing BooleanInterval(true, true)
    • I think [false, true] would be a better way of printing, in analogy with intervals [a, b] having a <= b. [0, 1] could be ok too.
    • Instead of creating the ad-hoc type, can't we just use Interval{Bool}(false, true)? (this causes a stack overflow when printing atm).
    • Another alternative to the previous point would be to just return Interval(0, 1). I think this might be the easiest option.
    • is this needed? (again YAGNI and KISS)
  • ternary logic: I think the promises you make "drop-in replacement with floats" and "allows composability" are a little too optimistic. There are a few points to be made:

    • intervals and real numbers don't follow the same axioms, e. g. x - x != 0, (yes it is true that floats don't obey either, but they are are somehow closer). The problem of comparisons that you fix here with ternary logic is one, but it's not the only one. With real numbers, the magnitude of a number is expressed by the absolute value, but for intervals you want the magnitude (or sometimes mignitude). A more elaborated example about this is here, with the ternary logic it would error instead of giving the wide interval, but I don't think it makes any better from a composability perspective.
    • Due to dependency problem and wrapping effect, the width of the intervals will grow during the computations and if you don't take this into account (e.g. use meanvalue form instead of natural enclosure, come up with a better algorithm, start paving) you will end up with non-informative results
    • Due to the above point, as the intervals width grows, the probability of them overlapping (and hence returns missing) grows too. If you have tiny intervals (idk, uncertainty max 1%), then this effect will remain mild and the intervals maybe disjoint in which case this works nicely (if they are disjoint, comparisons like in the standard are fine too, but of course it's best to have a panic switch for the rare cases in which it fails).
    • To be clear, I'm not saying it's bad, I think it's a nice addition, but the docs should be more honest in what it can do, that is, drop-in replacement and composition will work only for tiny intervals modeling small uncertainty, and depending on how big algorithm you apply to the interval, the biggest allowed uncertainty could be very small. (I wouldn't say it's composable if it errors 90% of the times)
    • It might probably be better to have the standard-compliant flavor as default
  • The ≛ symbol: I think I understand the reason why you introduced the extra symbol. With this PR == would be flavor dependent, but since we need to check equality of intervals (the way defined by the standard) internally, we definitely don't want the interval behavior to depend on what flavor a user changes (or is it? would it still be consistent to use == internally?). My main concerns with the symbol are:

    • it's a little exoteric, maybe something like or could be more intuitive.
    • As Luis pointed out, using === could be more Julian. I don't think the different behavior for decorated intervals is a problem, because internally the decoration is handled separately anyway. My main concern is that Interval{Float64}(1, 2) === Interval{Float32}(1, 2) evaluates to false. If it's safe to assume that both intervals are promoted to the same "numeric type" before they reach the function, then I think it's ok to use that, otherwise if == is flavor dependent an extra function/symbol for interval equality internally may be inevitable 🤔
  • For testing, I'd definitely say === is better, this way we don't have to check for the decoration separately. I'll check in ITF1788.jl if the +0.0 vs -0.0 thingy is now fixed by Luis' last commit, if it is I'd say we use that in testing.

  • Type system: I noticed at the moment DecoratedInterval doesn't have a supertype, I think it would be better to have

AbstractInterval <: Real # current situation, not saying I agree :)
Interval <: AbstractInterval
DecoratedInterval <: AbstractInterval
  • Structure: Realistically speaking, how many flavors will we have and how much will they differ from each other? An alternative approach to redefining methods based on the flavor would be to have an IntervalArithmeticBase.jl package with only the generic interface (and maybe the implementation of the methods that are for sure flavor independent if any) and then each flavor with its separate implementations in a separated package. This is the approach used in JuliaAlgebra where MultivariatePolynomials.jl only implements the common interface and then all the DynamicPolynomials, TypedPolynomials, StaticPolynomials etc. implement their methods their own way. I think also in SciML they have a similar approach (although I don't know any better what they do there). The advantage would be smaller startup time, no global state defining the flavor (solved the extra symbol issue). If we go this way, we could also ask the person behind NumberIntervals.jl if they are interested and have that package build on IntervalArithmeticBase.jl as the number-like flavor (as far as I can tell, the ternary logic is basically reimplementing that). I don't know, maybe it's a bad idea, open for feedback on this.

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.
Copy link
Member

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.

Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Member

@lucaferranti lucaferranti Jan 23, 2022

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."))
Copy link
Member

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)
Copy link
Member

@lucaferranti lucaferranti Jan 23, 2022

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:
Copy link
Member

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
Copy link
Member

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)
Copy link
Member

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
Copy link
Member

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)
Copy link
Member

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)

@lbenet
Copy link
Member

lbenet commented Jan 24, 2022

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 PointwisePolitic is part of the flavor system, which is already expressed by Benoît's comment: Julia is better at composability than the standard and we are more ambitious. In my (current) view, the PointwisePolitic defines a sub-flavor system, where for instance the meaning of < may differ, despite of the common set-based interpretation. So something I've been doing is to define the PointwisePolitic corresponding to the standard (I suggested to label it :ieee1788, but this can be changed to a better name later) so the ITF1788 test suite passes. (Currently, most tests pass locally using Julia 1.7.1, and those which don't is due to rounding issues, which I hope to finish soon.)

Regarding the design, which I like very much, there is something that is currently uncomfortable: If I need to change for example the PointwisePolitic (for instance, so the ITF1788 tests pass with the proper mode), I need to change the internal function IntervalArithmetic.pointwise_politic() in src/intervals/interval_operations/pointwise_boolean.jl, relaunch julia, recompile the package, and then use it. It would be fantastic to be able to do this at runtime, for which I guess we need something like Cassette, or the old tricks we used. This is not the most needed change at this point, but I mention it to have it in the to-do list.

@lucaferranti
Copy link
Member

lucaferranti commented Jan 24, 2022

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 f(::Setting, a, b) and then re-evaluate the default one whenever someone calls config. Then just call config() in the IntervalArithmetic.jl to set the default behavior when one loads the package the first time. This is the mechanism I use in ILA for setting the matrix multiplication algorithm. I think the current arrangement could also be problematic for dependent packages if they need the non-default pointwise policy.

@lbenet
Copy link
Member

lbenet commented Jan 24, 2022

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.

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.

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.

I agree. So this is what I've done: check the way that each file is included in the package, and go through the code (in Github and locally). Certainly not the best way of working, but I'm an old-er wolf 😄

Finally, I like the idea of the config function!

@Kolaru
Copy link
Collaborator Author

Kolaru commented Jan 25, 2022

@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.

  • all-in policy: Your docstring sounds a little judgy towards this policy and I agree with you. It inherits all downside of interval comparisons and is not compliant to the standard, so do we even need this?

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?

  • interval: I think this should be renamed to interval-boolean or something. As someone who also works with fuzzy logic, I like this, a few suggestions:

First, I am very open to name suggestions :D.

Then for the other points, here was my reasoning:

  • Define a new type that can be represented by 2 bits. Since there is only 3 possibilities, [false, true], [false] and [true], 2 bits is enough.
  • Do not use Interval{Bool} because it could make the method definition more complicated. Mostly because Bool <: Real and often the functions are define for Interval{T<:Real}.
  • I think it is good to have: it is the safest option since all conditional will just error unless specifically designed to handle intervals.
  • ternary logic: I think the promises you make "drop-in replacement with floats" and "allows composability" are a little too optimistic. There are a few points to be made:

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:

  1. You got a missing and you program errors.
  2. You got entireinterval() and the result is useless.

While with the current behavior, the worst case is: your results are wrong.

So the :ternary politic is a drop-in replacement only in the sense that it never gives you incorrect results, and only errors when needed. As you correclty points out, there is no guarantee the generic result will be any usefull of usable.

  • The ≛ symbol: I think I understand the reason why you introduced the extra symbol.

I should have been more explicit about that: I also think that === should be correct, but I wasn't absolutely sure. So I choose a symbol that I can search and replace in all file without problem in one go. That way we can modify it as we please once we agree on something (switching from == was a nightmare since the symbol is also used with non-interval).

  • Structure: Realistically speaking, how many flavors will we have and how much will they differ from each other?

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 0 * (-Inf..Inf) === (0..0).

Concerning pointwise politic, we probably only need one. I added multiple because it was easy enough and showed how to manage them.

An alternative approach to redefining methods based on the flavor would be to have an IntervalArithmeticBase.jl package with only the generic interface

I have no strong opinion on whether to use the type system or a global state for that. The arguments were essentially as follow:

  • In favor of using one interval type per flavor: we are using Julia so it is the most natural thing to do.
  • In favor of a global setting: it is the way describe in the standard. Moreover mixing intervals of different flavors doesn't make sense, so enforcing that only one flavor is used at any time is reasonnable.

If we go for the former, we will have to decide if we want the flavor to be a type parameter i.e. Interval{F<:Flavor, T}, or if each flavor should define its own type i.e. MyFlavor <: AbstractInterval. For the case you are describing, the second fits best.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Jan 25, 2022

If I need to change for example the PointwisePolitic (for instance, so the ITF1788 tests pass with the proper mode), I need to change the internal function IntervalArithmetic.pointwise_politic() in src/intervals/interval_operations/pointwise_boolean.jl, relaunch julia, recompile the package, and then use it.

Normally just typing IntervalArithmetic.pointwise_politic() == PointwisePolitic{:interval}() in the REPL or your script should work.

@Kolaru
Copy link
Collaborator Author

Kolaru commented Jan 25, 2022

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.

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:

  • Flavor/Pointwise politic.
  • The new parser (I rewrote it because I couldn't understand the current one).
  • The new behavior for ...

@lucaferranti
Copy link
Member

lucaferranti commented Jan 25, 2022

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

  • master
2×2 Matrix{Interval{Float64}}:
 [1, 1]  [1, 1]
 [2, 2]  [3, 3]
  • this PR
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?

@Kolaru
Copy link
Collaborator Author

Kolaru commented Jan 25, 2022

Why was the idea behind removing the promotion mechanism?

It caused hard to understand bug. Generally either StackOverflow or using a generic function defined for Real instead of a specific one (only if we forgot to implement it really). Except for array creation (that I didn't think of) it didn't seem to give great benefit in return.

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)

@lucaferranti
Copy link
Member

lucaferranti commented Jan 25, 2022

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

@lbenet
Copy link
Member

lbenet commented Feb 17, 2022

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.

@lucaferranti
Copy link
Member

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)

@Kolaru
Copy link
Collaborator Author

Kolaru commented Aug 21, 2023

Historically interested, and now superseeded by other developements.

@Kolaru Kolaru closed this Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants