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

nthroot and rational power #533

Merged
merged 5 commits into from
Jun 1, 2022
Merged

nthroot and rational power #533

merged 5 commits into from
Jun 1, 2022

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented May 28, 2022

The idea behind this PR is to get the same results for a^(1//n) and nthroot(a, n) for n::Integer, when the comparison makes sense (there are small differences, which are related to a). This allows to pass a bunch of tests in IntervalContractors.jl, after some tweeks (see JuliaIntervals/IntervalContractors.jl#55).

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2022

Codecov Report

Merging #533 (e3c21f0) into 1.0-dev (cafc327) will decrease coverage by 0.22%.
The diff coverage is 93.93%.

@@             Coverage Diff             @@
##           1.0-dev     #533      +/-   ##
===========================================
- Coverage    85.44%   85.22%   -0.23%     
===========================================
  Files           34       34              
  Lines         1814     1793      -21     
===========================================
- Hits          1550     1528      -22     
- Misses         264      265       +1     
Impacted Files Coverage Δ
src/intervals/arithmetic/power.jl 96.05% <93.93%> (-1.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cafc327...e3c21f0. Read the comment docs.

@lucaferranti
Copy link
Member

there are small differences, which are related to a

What differences? I think nthroot and ^(1//n) should be identical🤔

@lbenet
Copy link
Member Author

lbenet commented May 29, 2022

I agree with the comment, indeed, they should behave the same. Yet, we are speaking about the power function, which means the possibility to different interpretations related to the actual a. An example:

julia> nthroot( -27 .. 27, 3)
[-3, 3]

julia> (-27 .. 27)^(1//3)
[0, 3]

This PR doesn't (yet) address these differences, but tries to ensure that the results are the same when they should be.

@lucaferranti
Copy link
Member

lucaferranti commented May 29, 2022

Very good example! Now I actually think they should not be the same, as they have different domains and codomains. This is also highlighted comparing the definition of pow on table 9.1 to the one of rootn on table 10.5.

@lbenet
Copy link
Member Author

lbenet commented May 29, 2022

Thanks for pointing out those tables; I'll check them. I also think that we can now reduce the code duplication.

@lbenet lbenet added the 1.0 Planned for the major 1.0 release label May 29, 2022
@lbenet lbenet force-pushed the lb/nthroot_rationalpow branch from 260b4eb to e99f547 Compare May 29, 2022 14:38
@lbenet
Copy link
Member Author

lbenet commented May 29, 2022

This should fix the problems in JuliaIntervals/IntervalContractors.jl#55; once this is merged to 1.0-dev branch, tests should pass there...

@lbenet lbenet requested a review from lucaferranti May 29, 2022 19:18
Copy link
Member

@lucaferranti lucaferranti left a comment

Choose a reason for hiding this comment

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

@lbenet in general LGTM. I left two very minor comments.

As a more general comment, have you looked at the current tests for nthroot and rational power? Are there some corner cases which this PR fixes that should be added to the tests?

I checked in IntervalContractors and with this branch there are "only" 233 broken tests in pow_rev, which are presumably some tricky cases.

src/intervals/arithmetic/power.jl Show resolved Hide resolved
src/intervals/arithmetic/power.jl Show resolved Hide resolved
@lbenet
Copy link
Member Author

lbenet commented Jun 1, 2022

I checked in IntervalContractors and with this branch there are "only" 233 broken tests in pow_rev, which are presumably some tricky cases.

Could you tell me which tests are still not passing? I admit that I did not checked where are the broken tests, though I saw the 233...

@lucaferranti
Copy link
Member

Could you tell me which tests are still not passing? I admit that I did not checked where are the broken tests, though I saw the 233...

The ones marked as @test_broken in your lb/power_rev branch :) With a quick look, it seems that most (if not all) of these tests involve unbounded intervals, so maybe the issue is how we handle infinities

@lucaferranti
Copy link
Member

lucaferranti commented Jun 1, 2022

The ones marked as @test_broken in your lb/power_rev branch :) With a quick look, it seems that most (if not all) of these tests involve unbounded intervals, so maybe the issue is how we handle infinities

but that's very likely to be an inssue in IntervalContractors, because atm there are no checks or special handling, the functions are just

function pow_rev1(b, c, x)   # c = x^b
    return x  c^(1/b) 
end

function pow_rev2(a, c, x)   # c = a^x
    return x  (log(c) / log(a))
end

my point simply being that this PR can be merged with clean consciousness even if tests in IntervalContractors still fail

@lbenet
Copy link
Member Author

lbenet commented Jun 1, 2022

my point simply being that this PR can be merged with clean consciousness even if tests in IntervalContractors still fail

I agree to merge this, at least to make it easier to see what's going on in IntervalContractors...

Incidentally, I did not touch power_rev1 or power_rev2, which maybe better to do in a separate PR

@lbenet
Copy link
Member Author

lbenet commented Jun 1, 2022

Going ahead and merging!

@lbenet lbenet merged commit 1d2e6e4 into 1.0-dev Jun 1, 2022
@lbenet lbenet deleted the lb/nthroot_rationalpow branch June 1, 2022 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Planned for the major 1.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants