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.

Sign up for GitHub

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

Jump to bottom

fixed bugs towards ITF1788 compliance #49

Merged
merged 11 commits into from
Feb 4, 2022
Merged

fixed bugs towards ITF1788 compliance #49

merged 11 commits into from
Feb 4, 2022

Conversation

lucaferranti
Copy link
Member

@lucaferranti lucaferranti commented May 31, 2021 •
edited
Loading

  • fixed typo in pow_rev2 (lob -> log
  • fixed power_rev for exponent zero integer
  • added support for decorated intervals
  • added methods with one less parameter with x=R
  • fixed cosh_rev
  • added mul_rev_to_pair
  • added tests from ITF1788 test suite

remaining issues

ieeep1788_rev.itl

  • 16 tests with power_rev failing. From what I can tell, the returned result is just a few ulps wider than the expected one.
  • 33 tests failing with mul_rev_IEEE1788, but haven't had time to look at those yet

pow_rev.itl

  • at the moment 233/806 tests failing. Probably the current implementations do not take into account some corner cases? Need to check those

@lucaferranti
fixed bugs towards ITF1788 compliance
44413c5 
* fixed typo in po_rev2
* fixed power_rev for exponent zero integer
* added support for decorated intervals
* added methods with one less parameter with x=R
* fixed cosh_rev
* added mul_rev_to_pair
* added tests from ITF1788 test suite
@lucaferranti lucaferranti mentioned this pull request May 31, 2021
Open
11 tasks
@lucaferranti lucaferranti marked this pull request as draft May 31, 2021 19:39
@lucaferranti lucaferranti marked this pull request as ready for review January 26, 2022 15:17
@lucaferranti
Copy link
Member Author

@lbenet most ITF1788 tests were failing for reverse functions, this PR fixes most of them. The remaining one are other tedious (rev_pow, there's a whole phd dissertation on how to compute that) or probably difficult (power_rev in some cases returns slightly wider results). I suggest to merge this now and track the remaining issues in an issue for later.

(As I briefly mentioned yesterday, I think going through rev_pow and implement it could be a suitable project for an undergrad with some interest in interval methods)

@lucaferranti
Copy link
Member Author

remaining things to do tracked in #48

@lbenet
Copy link
Member

lbenet commented Jan 26, 2022

@lucaferranti I'll try to find some space this afternoon to review this PR; today I have a very busy morning...

Aside from that, wouldn't this be better in IntervalAithmetic.jl package, precisely for compliance with the standard? This package at the end relies on IA, so there is no new dependency.

@lucaferranti
Copy link
Member Author

lucaferranti commented Jan 26, 2022 •
edited
Loading

I don't have an opinion about whether the reverse functions should be here or in IA, from a standard compliance it may make more sense to have those in IA. Alternatively, one can sell ValidatedNumerics as "the" package.

One thing that could be nice to do in this package is to define a common contractor interface for both IntervalRootFinding.jl and IntervalConstraintProgramming, e.g. currently both packages export Contractor, which is a little suboptimal. However this can be discussed later I guess.

I think @dpsanders had some reasons to have reverse functions in a separate package?

lbenet
lbenet reviewed Jan 26, 2022
View reviewed changes
Copy link
Member

@lbenet lbenet left a comment

Choose a reason for hiding this comment

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

I've left essentially two comments:

  • one related to the generation of the rev functions when the last interval argument omitted (is optional), which I think should include all the fnctions in the compacted code
  • one (actually two) related with the docstrings

Since this PR impacts (I think) JuliaIntervals/IntervalArithmetic.jl#505, I suggest to take into account the first comment and merge this PR, and leave the second comment to another PR. Once said this, apart from that comment, LGTM.

src/arithmetic.jl Outdated
Comment on lines 90 to 93
"""
Reverse power
"""
function power_rev(a::Interval, b::Interval, n::Integer) # a = b^n, log(a) = n.log(b), b = a^(1/n)
function power_rev(a::Interval{T}, b::Interval{T}, n::Integer) where T # a = b^n, log(a) = n.log(b), b = a^(1/n)
Copy link
Member

Choose a reason for hiding this comment

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

Since there are various "powRev" functions specified in the standard (cf. Sect 10.5.4 and Table 10.1), it's important to have very descriptive docstrings, in this case, of the specific function in the standard, or the differences with that.

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 comment applies to other functions...

src/decorated.jl Show resolved Hide resolved
@lucaferranti
fixed bugs towards ITF1788 compliance
75ab7c5 
* fixed typo in po_rev2
* fixed power_rev for exponent zero integer
* added support for decorated intervals
* added methods with one less parameter with x=R
* fixed cosh_rev
* added mul_rev_to_pair
* added tests from ITF1788 test suite
@lucaferranti
fix mul_rev_IEEE
8068bab
@lucaferranti
added docstrings for power_rev, pow_rev1, pow_rev2
4371384
@lucaferranti
updated tests
f3f0884
@lucaferranti
fixed conflicts
959e789
@lucaferranti
Copy link
Member Author

@lbenet I've updated some docstrings (the three for powers to disambiguate and a few more), if you think the template is good, I can add to the others too (maybe in a separate PR, if the idea was to merge this before the other PR in IntervalArithmetic.jl).

I didn't quite understand your other comment, the power_rev function with one parameter seems to be defined here

@lbenet
Copy link
Member

lbenet commented Jan 29, 2022

In view of JuliaIntervals/IntervalArithmetic.jl#271, my only suggestion is to add the reference section of the standard where the rev functions are described; I think this is Sect 10.5.4 and Table 10.1. But this can be included in a separated PR.

@lucaferranti
Copy link
Member Author

@lbenet apologies for my delay in coming back to this.

I have now clarified the docstrings for the functions with reference to the standard. Let me know if you want me to do other changes, otherwise once this is merged I can update JuliaIntervals/IntervalArithmetic.jl#505

lbenet
lbenet approved these changes Feb 4, 2022
View reviewed changes
Copy link
Member

@lbenet lbenet left a comment

Choose a reason for hiding this comment

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

LGTM! In another PR we may add more descriptive docstrings (with regards to all rev functions and their mention in the standard). Thanks a lot!

@lbenet
Copy link
Member

lbenet commented Feb 4, 2022

Please, go ahead and merge this!

@lucaferranti lucaferranti merged commit bb96de5 into JuliaIntervals:master Feb 4, 2022
@lucaferranti lucaferranti deleted the lf/1788-compliance branch February 4, 2022 19:49
@lucaferranti lucaferranti mentioned this pull request Jun 2, 2022
Closed
@lucaferranti lucaferranti mentioned this pull request Jun 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbenet lbenet lbenet approved these changes

@dpsanders dpsanders Awaiting requested review from dpsanders

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lucaferranti @lbenet @JuliaRegistrator