-
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
Simplified version of mod(x::Interval, y::Real) #525
Conversation
Btw, I've also found that julia> using IntervalArithmetic
julia> function mod178(X::Interval, a::Interval)
division = X / a
fl = floor(division)
if fl.lo < fl.hi
return 0..(a.hi)
end
return (division - fl) * a
end
mod178 (generic function with 1 method)
julia> mod178(1..1, 0.9..1.1)
[0, 1.10001]
julia> maximum(mod(1, x) for x in 0.9:0.01:1.1)
1.0 |
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #525 +/- ##
==========================================
+ Coverage 90.36% 90.96% +0.59%
==========================================
Files 25 25
Lines 1796 1837 +41
==========================================
+ Hits 1623 1671 +48
+ Misses 173 166 -7
☔ View full report in Codecov by Sentry. |
Co-authored-by: lucaferranti <[email protected]>
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.
Thanks a lot for addressing #129.
In general the pull-request LGTM! I left some suggestions, which I think are easy to implement.
src/intervals/functions.jl
Outdated
Calculate `x mod y` where `x` is an interval and `y` is a positive divisor. | ||
""" | ||
function mod(x::Interval, y::Real) | ||
@assert y > zero(y) "modulo is currently implemented only for a positive divisor." |
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.
Is it possible to extend this to have strictly negative y
? I understand that having 0 ∈ y
complicates things....
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.
Updated for strictly negative divisor. Hope its correct.
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 implementation seems to me correct if we have y::AbstractFloat
(perhaps also including Rationals, but let me forget about them now), which corresponds to the tests.
However, if y::Interval
(and we have Interval <: Real
) then there this function throws errors: e.g., try mod(1..2, 1..2)
, which I think should return Interval(0.0, 2.0)
. In this case things are subtle, because y != zero(y)
is true for [-1, 1]
, but that interval is not strictly positive or negative. Also, Interval(zero(y), y)
causes the error mentioned above. I guess this is the reason that mod
has two methods in #178.
My suggestion is either restrict y::AbstractFloat
, or include a new method where y::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.
This is my mistake. My intention was to constrain y
NOT to be Interval
. I've not realized 1..3 isa Real -> true
. However, If I constraint it to y::AbstractFloat
it does not accept integers for example because 1 isa AbstractFloat -> false
, right?
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.
you could do Union{AbstractFloat, Integer}
to accept both integers and floats but no intervals (similar to add rationals and irrationals).
Can the method be generalized to the case of y
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.
actually forget about the union, a better suggestion would be to define
mod(x::Real, y::Interval) = throw(ArgumentError("mod not defined for second argument interval"))
mod(x::Interval, y::Interval) = throw(ArgumentError("mod not defined for second argument 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.
mod
can indeed be generalized for y::Interval
. It's tricky with respect of having zero within the interval, which is part of the reason I was suggesting to have either a strictly positive or negative y
. Actually, a motivating example would be to have y::Irrational
, or actually any (mathematical) real number which is not exactly representable as a Float64
. Note that #178 includes such an implementation, though it does not include the restriction of strictly positive/negative intervals. The subtleties related to zero at the end are related to the division: y
appears in the denominator.
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 unresolved this conversation, so it is easy to track the discussion...
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 in octave
>> mod(infsup(-3, 2), infsup(-1, 0.5))
ans = [-1, +0.5]
>> mod(infsup(-3, 2), infsup(0, 0))
ans = [Empty]
>> mod(infsup(-3, 2), infsup(0, 3))
ans = [0, 3]
>> mod(infsup(-3, 2), infsup(1, 3))
ans = [0, 3]
>> mod(infsup(-3, 2), infsup(-3, 2))
ans = [-3, +2]
Calculate `x::Interval mod y::Real`, limited by `y != 0`. | ||
""" | ||
function mod(x::Interval, y::Real) | ||
@assert y != zero(y) """mod(x::Interval, y::Real) |
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.
an alternative would be to return an empty interval, I think this would be more conformant with the IEEE standard behavior, e.g.
julia> sqrt(-1.. -1)
∅
julia> log(-2.. -1)
∅
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.
Or we can define mod(a..b, 0) == 0..0 ⊇ ∅
as Wolfram says
https://www.wolframalpha.com/input?i=limit+x-%3E0%2B+mod%281%2C+x%29
Consequently, for 0 ∈ y
, we can define smth like mod(x,y) = union(mod(x,(y.lo..y.lo/3)), mod(x, (y.hi/3)..(y.hi)))
because the maximum value needs to by in (y.hi/2)..(y.hi)
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 propose to solve this in some future PR, and move the discussion to #129, to keep track once this PR merged.
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.
Or we can define
mod(a..b, 0) == 0..0 ⊇ ∅
as Wolfram says https://www.wolframalpha.com/input?i=limit+x-%3E0%2B+mod%281%2C+x%29
Julia (and Wolfram) return NaN
(undefined
) for mod(3.2, 0.0)
, which makes sense due to the division in the algorithm.
Consequently, for
0 ∈ y
, we can define smth likemod(x,y) = union(mod(x,(y.lo..y.lo/3)), mod(x, (y.hi/3)..(y.hi)))
because the maximum value needs to by in(y.hi/2)..(y.hi)
This idea is interesting, and reminds me a little bit what we have for extended_div
. That certainly should be addressed in another PR. Yet, I think for (mathematical) reals, mod(x,y)
is a number in the interval [0,y] for y>0, or [y, 0] if y<0, isn't it?
From the range of mod(x,y)
for real y
, then for a strictly positive interval y
, we should return [0, sup(y)]
, or if it is strictly negative [inf(y), 0]
. If 0 ∈ y
, we should return the hull of these intervals.
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 my last comment corresponds to these results... Or not?
Sorry for the extended delay. Since all tests are passing and all discussions are passing, I am merging. Thanks a lot for your contribution ! |
commit 1c0522f Merge: 4c239d5 ee93bd4 Author: Benoît Richard <[email protected]> Date: Sun Jul 23 23:13:48 2023 +0200 Merge pull request #569 from vaerksted/master fix typos commit ee93bd4 Author: spaette <[email protected]> Date: Sat Jul 22 12:11:01 2023 -0500 typos commit 4c239d5 Author: Benoît Richard <[email protected]> Date: Tue Jul 4 00:35:20 2023 +0200 Bump version commit 249040d Merge: 4404658 7bef23e Author: Benoît Richard <[email protected]> Date: Tue Jul 4 00:34:13 2023 +0200 Merge pull request #525 from petvana/simplified-mod Simplified version of mod(x::Interval, y::Real) commit 4404658 Author: Benoît Richard <[email protected]> Date: Wed Jun 14 14:03:14 2023 +0200 Bump version for release commit 7bef23e Author: Petr Vana <[email protected]> Date: Fri May 27 11:11:53 2022 +0200 Cleanup commit f9f4733 Author: Petr Vana <[email protected]> Date: Fri May 27 11:10:05 2022 +0200 Throw ArgumentError for Interval divisor for mod commit 6fdc809 Merge: 439723f d2603d6 Author: Petr Vana <[email protected]> Date: Fri May 27 11:01:43 2022 +0200 Merge branch 'simplified-mod' of github.com:petvana/IntervalArithmetic.jl into simplified-mod commit 439723f Author: Petr Vana <[email protected]> Date: Fri May 27 11:01:27 2022 +0200 Disable divisor for mod to be an interval commit d2603d6 Author: Petr Vana <[email protected]> Date: Thu May 26 11:34:04 2022 +0200 Update docs commit 45abc46 Author: Petr Vana <[email protected]> Date: Thu May 26 11:30:14 2022 +0200 Implementation for strictly negative divisors for mod commit ff47910 Author: Petr Vana <[email protected]> Date: Thu May 26 11:11:35 2022 +0200 Add todo for mod with between two intervals commit ea00b23 Author: Petr Vana <[email protected]> Date: Wed May 25 19:59:30 2022 +0200 Imrpove test coverage + use zero() commit b56f4be Author: Petr Vana <[email protected]> Date: Wed May 25 13:44:24 2022 +0200 Use ⊇ operator commit d23df89 Author: Petr Vana <[email protected]> Date: Tue May 24 21:13:53 2022 +0200 Update src/intervals/functions.jl Co-authored-by: lucaferranti <[email protected]> commit 153d749 Author: Petr Vana <[email protected]> Date: Tue May 24 20:27:51 2022 +0200 Improve testing commit fea1b34 Author: Petr Vana <[email protected]> Date: Tue May 24 20:10:05 2022 +0200 Introduce simplified version of mod
commit 1c0522f Merge: 4c239d5 ee93bd4 Author: Benoît Richard <[email protected]> Date: Sun Jul 23 23:13:48 2023 +0200 Merge pull request #569 from vaerksted/master fix typos commit ee93bd4 Author: spaette <[email protected]> Date: Sat Jul 22 12:11:01 2023 -0500 typos commit 4c239d5 Author: Benoît Richard <[email protected]> Date: Tue Jul 4 00:35:20 2023 +0200 Bump version commit 249040d Merge: 4404658 7bef23e Author: Benoît Richard <[email protected]> Date: Tue Jul 4 00:34:13 2023 +0200 Merge pull request #525 from petvana/simplified-mod Simplified version of mod(x::Interval, y::Real) commit 4404658 Author: Benoît Richard <[email protected]> Date: Wed Jun 14 14:03:14 2023 +0200 Bump version for release commit 7bef23e Author: Petr Vana <[email protected]> Date: Fri May 27 11:11:53 2022 +0200 Cleanup commit f9f4733 Author: Petr Vana <[email protected]> Date: Fri May 27 11:10:05 2022 +0200 Throw ArgumentError for Interval divisor for mod commit 6fdc809 Merge: 439723f d2603d6 Author: Petr Vana <[email protected]> Date: Fri May 27 11:01:43 2022 +0200 Merge branch 'simplified-mod' of github.com:petvana/IntervalArithmetic.jl into simplified-mod commit 439723f Author: Petr Vana <[email protected]> Date: Fri May 27 11:01:27 2022 +0200 Disable divisor for mod to be an interval commit d2603d6 Author: Petr Vana <[email protected]> Date: Thu May 26 11:34:04 2022 +0200 Update docs commit 45abc46 Author: Petr Vana <[email protected]> Date: Thu May 26 11:30:14 2022 +0200 Implementation for strictly negative divisors for mod commit ff47910 Author: Petr Vana <[email protected]> Date: Thu May 26 11:11:35 2022 +0200 Add todo for mod with between two intervals commit ea00b23 Author: Petr Vana <[email protected]> Date: Wed May 25 19:59:30 2022 +0200 Imrpove test coverage + use zero() commit b56f4be Author: Petr Vana <[email protected]> Date: Wed May 25 13:44:24 2022 +0200 Use ⊇ operator commit d23df89 Author: Petr Vana <[email protected]> Date: Tue May 24 21:13:53 2022 +0200 Update src/intervals/functions.jl Co-authored-by: lucaferranti <[email protected]> commit 153d749 Author: Petr Vana <[email protected]> Date: Tue May 24 20:27:51 2022 +0200 Improve testing commit fea1b34 Author: Petr Vana <[email protected]> Date: Tue May 24 20:10:05 2022 +0200 Introduce simplified version of mod
This PR cherry-picks
mod(x::Interval, y::Real)
from #178 because it seems to be the most useful case (at least for me), and also easiest to implement.Cc: @dpsanders @lbenet
fixes #129