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

Implemented clamp(t, i::ClosedInterval) #77

Merged
merged 1 commit into from
May 4, 2021
Merged

Conversation

jagot
Copy link
Member

@jagot jagot commented Apr 13, 2021

With this PR, we can do

julia> using IntervalSets

julia> clamp(0.4, 1..4)
1.0

I am unsure if clamp makes sense for non-closed intervals, so I only
implemented it for ClosedInterval.

@codecov
Copy link

codecov bot commented Apr 13, 2021

Codecov Report

Merging #77 (3109820) into master (4e5e2e9) will increase coverage by 0.30%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
+ Coverage   90.63%   90.94%   +0.30%     
==========================================
  Files           3        3              
  Lines         267      265       -2     
==========================================
- Hits          242      241       -1     
+ Misses         25       24       -1     
Impacted Files Coverage Δ
src/IntervalSets.jl 82.03% <100.00%> (ø)
src/interval.jl 99.08% <0.00%> (+0.88%) ⬆️

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 4e5e2e9...3109820. Read the comment docs.

@dlfivefifty
Copy link
Member

dlfivefifty commented Apr 14, 2021

Can you add a test for broadcasting? I'd expect

  julia> clamp.([pi, 1.0, big(10.)], 2..9.)
  3-element Vector{BigFloat}:
   3.141592653589793238462643383279502884197169399375105820974944592307816406286198
   2.0
   9.0

though perhaps it's not clear how broadcasting would work...

@jagot
Copy link
Member Author

jagot commented Apr 14, 2021

At the moment, that's an error:

julia> clamp.([pi, 1.0, big(10.)], 2..9.)
ERROR: MethodError: no method matching iterate(::ClosedInterval{Float64})
Closest candidates are:
  iterate(::Union{LinRange, StepRangeLen}) at range.jl:664
  iterate(::Union{LinRange, StepRangeLen}, ::Int64) at range.jl:664
  iterate(::T) where T<:Union{Base.KeySet{var"#s79", var"#s78"} where {var"#s79", var"#s78"<:Dict}, Base.ValueIterator{var"#s77"} where var"#s77"<:Dict} at dict.jl:693
  ...
Stacktrace:
 [1] _collect(cont::UnitRange{Int64}, itr::ClosedInterval{Float64}, #unused#::Base.HasEltype, isz::Base.SizeUnknown)
   @ Base ./array.jl:613
 [2] collect(itr::ClosedInterval{Float64})
   @ Base ./array.jl:602
 [3] broadcastable(x::ClosedInterval{Float64})
   @ Base.Broadcast ./broadcast.jl:682
 [4] broadcasted(::Function, ::Vector{BigFloat}, ::ClosedInterval{Float64})
   @ Base.Broadcast ./broadcast.jl:1313
 [5] top-level scope
   @ REPL[9]:1

which of course can be solved by

julia> Base.Broadcast.broadcastable(i::ClosedInterval) = Ref(i)

julia> clamp.([pi, 1.0, big(10.)], 2..9.)
3-element Vector{BigFloat}:
 3.141592653589793238462643383279502884197169399375105820974944592307816406286198
 2.0
 9.0

I don't know if that opens a can of worms or not. Should the broadcastable overload be at the TypedEndpointsInterval level? Note that it would be in direct conflict with #32 & #55, i.e. after the overload above we now get

julia> (2..9) .+ 3
ERROR: MethodError: no method matching +(::ClosedInterval{Int64}, ::Int64)
Closest candidates are:
  +(::Any, ::Any, ::Any, ::Any...) at operators.jl:560
  +(::T, ::T) where T<:Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8} at int.jl:87
  +(::Rational, ::Integer) at rational.jl:288
  ...
Stacktrace:
 [1] _broadcast_getindex_evalf
   @ ./broadcast.jl:648 [inlined]
 [2] _broadcast_getindex
   @ ./broadcast.jl:621 [inlined]
 [3] getindex
   @ ./broadcast.jl:575 [inlined]
 [4] copy
   @ ./broadcast.jl:898 [inlined]
 [5] materialize(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{0}, Nothing, typeof(+), Tuple{Base.RefValue{ClosedInterval{Int64}}, Int64}})
   @ Base.Broadcast ./broadcast.jl:883
 [6] top-level scope
   @ REPL[12]:1

instead of

julia> (2..9.) .+ 1
ERROR: MethodError: no method matching iterate(::ClosedInterval{Float64})
Closest candidates are:
  iterate(::Union{LinRange, StepRangeLen}) at range.jl:664
  iterate(::Union{LinRange, StepRangeLen}, ::Int64) at range.jl:664
  iterate(::T) where T<:Union{Base.KeySet{var"#s79", var"#s78"} where {var"#s79", var"#s78"<:Dict}, Base.ValueIterator{var"#s77"} where var"#s77"<:Dict} at dict.jl:693
  ...
Stacktrace:
 [1] _collect(cont::UnitRange{Int64}, itr::ClosedInterval{Float64}, #unused#::Base.HasEltype, isz::Base.SizeUnknown)
   @ Base ./array.jl:613
 [2] collect(itr::ClosedInterval{Float64})
   @ Base ./array.jl:602
 [3] broadcastable(x::ClosedInterval{Float64})
   @ Base.Broadcast ./broadcast.jl:682
 [4] broadcasted(::Function, ::ClosedInterval{Float64}, ::Int64)
   @ Base.Broadcast ./broadcast.jl:1312
 [5] top-level scope
   @ REPL[4]:1

without the overload.

For now, I've added the test case clamp.([pi, 1.0, big(10.)], Ref(2..9.)) instead.

@dlfivefifty
Copy link
Member

I think forget broadcasting for now

@jagot
Copy link
Member Author

jagot commented Apr 14, 2021

So, merge as-is?

@jagot
Copy link
Member Author

jagot commented May 4, 2021

Bump 🙂

@dlfivefifty dlfivefifty merged commit e73b682 into master May 4, 2021
@hyrodium hyrodium deleted the jagot/clamp branch March 31, 2022 08:11
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.

2 participants