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

range(; stop) and range(; length). Single keyword args only. Single pos arg not allowed. #39241

Merged
merged 3 commits into from
Aug 16, 2021

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Jan 14, 2021

range(; stop), range(; length)

This is a minimalist version of #39223 to expose Base.OneTo when a single keyword argument of range is given. It only allows either stop or length to be provided as the sole keyword argument. In that circumstance only, start, and step are assumed to be one.

It does not allow for one positional argument to be given.

# Synposis
range(; stop) = 1:stop
range(; stop::Integer) = Base.OneTo(stop)
range(; length::Integer) = Base.OneTo(length) 
# The above is for explanatory purposes only and not actual code added

Usage:

julia> range(; stop = 5.5)
1.0:1.0:5.0

julia> range(; stop = 5)
Base.OneTo(5)

julia> range(; length = 6)
Base.OneTo(6)

julia> range(5) # One positional argument is not allowed
ERROR: ArgumentError: Cannot construct range from arguments:
...

julia> range(; stop=10, length=5) # step is assumed to be 1, no impact from this PR
6:10

julia> range(; step=1, length=5) # start is not assumed to be 1 when more than one keyword is given
ERROR: ArgumentError: Cannot construct range from arguments:
...

julia> range(; step=1, stop=6 )# start is not assumed to be 1 when more than one keyword is given
ERROR: ArgumentError: Cannot construct range from arguments:
...

Background on Base.OneTo

Base.OneTo is a highly optimized version of 1:n that uses the type system to guarantee the first element is one. While 1:n is very common in Julia, it should be replaced by Base.OneTo(n) in highly optimized code. This pull request attempts to make it more accessible via range.

julia> using BenchmarkTools

julia> o = Base.OneTo(1_000_000_000)
Base.OneTo(1000000000)

julia> r = 1:1_000_000_000
1:1000000000

julia> @btime $r[$o]
  2.499 ns (0 allocations: 0 bytes)
1:1000000000

julia> @btime $r[$r]
  4.299 ns (0 allocations: 0 bytes)
1:1000000000

References

This strictly implements

* Only `start` and `stop` are provided
* Only `length` and `stop` are provided

A `UnitRange` is not produced if `step` is provided even if specified as one.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the whole extended help section, it is very clear.

@mkitti mkitti changed the title range with a single keyword arg: range(; stop) or range(; length). Single pos arg not allowed. range(; stop) and range(; length). Single keyword args only. Single pos arg not allowed. Jan 18, 2021
@mkitti
Copy link
Contributor Author

mkitti commented Feb 17, 2021

@mbauman, could we add the triage tag to this pull request? This request just adds two keyword forms where only stop or length are specified. Importantly it also creates Base.range_stop and Base.range_length which can be used by a subsequent single positional argument pull request.

Single positional argument should be its own pull request

Adding a single positional argument on top of this could be as easy as range(stop::Integer) = range_stop(stop) or range(stop::Number) = range_stop(stop) but it could also be more complicated.

range(stop) = range_stop(stop) creates strange method signatures such as range(stop; stop, length, step):

julia> Base.range(stop) = Base.range_stop(stop)

julia> methods(range)
# 4 methods for generic function "range":
[1] range(; start, stop, length, step) in Base at range.jl:138
[2] range(stop; stop, length, step) in Main at REPL[5]:1
...

You could also dispatch from within the positional argument versions of the function if stop, length, and step are all nothing, but this creates a second dispatching structure for positional argument versions of range.

Single positional argument range not only has differences in opinion regarding its existence but also potential complications in terms of implementation. I personally lean towards the simpler forms. However, I think that needs its own discussion. Having this merged in first simplifies that discussion.

Avoiding the ambiguity of whether step = 1 or start = 1.

As you noted in #39223 (comment), there is an ambiguity between the assumption model of either step = 1 or start = 1. This avoids that ambiguity by focusing on the case when step = 1 and start = 1.

Future pull request plans

After this, I would proceed with a pull request for range(stop::Integer) = range_stop(stop). This would match the similar Python call which only accepts integers. We could then discuss there whether that alone is sufficient or if more is needed.

@mbauman
Copy link
Sponsor Member

mbauman commented Feb 23, 2021

Speaking personally, I'm rather lukewarm about these final steps, and this hasn't gotten much engagement from others. There's a tradeoff here in the complexity of usage and documentation, and I don't see the new functionality significantly outweighing these costs.

I'd be inclined to let the range changes simmer a bit — let's live with the new functionality for a release or two and see how it feels before doing more.

@mkitti
Copy link
Contributor Author

mkitti commented Feb 23, 2021

That's fine with me. I was under the impression that we did not want space this out over many releases.

@baggepinnen
Copy link
Contributor

Since

julia> Base.OneTo(3) isa UnitRange
false

It's worth to consider all the places in julia as well as in the ecosystem where method arguments are restricted to UnitRange, e.g.,

julia> methodswith(typeof(1:3))
[1] getindex(A::Array, I::UnitRange{Int64}) in Base at array.jl:805
[2] getindex(s::LaTeXStrings.LaTeXString, i::UnitRange{Int64}) in LaTeXStrings at /home/fredrikb/.julia/packages/LaTeXStrings/YQ4GM/src/LaTeXStrings.jl:112
[3] getindex(s::String, r::UnitRange{Int64}) in Base at strings/string.jl:257
[4] setindex!(A::Array{T, N} where N, X::Array{T, N} where N, I::UnitRange{Int64}) where T in Base at array.jl:860
[5] setindex!(B::BitArray, X::Union{BitArray, StridedArray{T, N} where {T, N}}, J0::Union{Colon, UnitRange{Int64}}) in Base at multidimensional.jl:1363
[6] setindex!(B::BitArray, X::Union{BitArray, StridedArray{T, N} where {T, N}}, I0::Union{Colon, UnitRange{Int64}}, I::Union{Colon, UnitRange{Int64}, Int64}...) in Base at multidimensional.jl:1374
[7] setindex!(B::BitArray, X::AbstractArray, J0::Union{Colon, UnitRange{Int64}}) in Base at bitarray.jl:706
[8] setindex!(B::BitArray, X::AbstractArray, I0::Union{Colon, UnitRange{Int64}}, I::Union{Colon, UnitRange{Int64}, Int64}...) in Base at multidimensional.jl:1415
[9] splice!(B::BitVector, r::Union{UnitRange{Int64}, Integer}) in Base at bitarray.jl:1034
[10] splice!(B::BitVector, r::Union{UnitRange{Int64}, Integer}, ins::AbstractArray) in Base at bitarray.jl:1034
[11] splice!(B::BitVector, r::Union{UnitRange{Int64}, Integer}, ins) in Base at bitarray.jl:1076
[12] deleteat!(B::BitVector, r::UnitRange{Int64}) in Base at bitarray.jl:957
[13] copy_transpose!(B::AbstractMatrix{T} where T, ir_dest::UnitRange{Int64}, jr_dest::UnitRange{Int64}, tM::AbstractChar, M::AbstractVecOrMat{T} where T, ir_src::UnitRange{Int64}, jr_src::UnitRange{Int64}) in LinearAlgebra at /home/fredrikb/julia-1.6.0-rc1/share/julia/stdlib/v1.6/LinearAlgebra/src/matmul.jl:691
[14] copyto!(B::AbstractVecOrMat{T} where T, ir_dest::UnitRange{Int64}, jr_dest::UnitRange{Int64}, tM::AbstractChar, M::AbstractVecOrMat{T} where T, ir_src::UnitRange{Int64}, jr_src::UnitRange{Int64}) in LinearAlgebra at /home/fredrikb/julia-1.6.0-rc1/share/julia/stdlib/v1.6/LinearAlgebra/src/matmul.jl:681

Changing the return type of range might not be ideal from this perspective.

@mbauman
Copy link
Sponsor Member

mbauman commented Feb 25, 2021

I wouldn't consider that a show-stopper. This isn't changing any return types. It's enabling new methods that have new behaviors. Even if it did change return types, I wouldn't necessarily consider it a show stopper; it'd just be one more thing to weigh the new functionality against.

@mkitti
Copy link
Contributor Author

mkitti commented Feb 25, 2021

It's worth to consider all the places in julia as well as in the ecosystem where method arguments are restricted to UnitRange

That's an important consideration. Note that Base.OneTo is an AbstractUnitRange. It may just a matter of widening some of those methods to AbstractUnitRange rather than UnitRange. Conversion is not very difficult either.

julia> UnitRange <: AbstractUnitRange
true

julia> Base.OneTo <: AbstractUnitRange
true

julia> UnitRange(Base.OneTo(3))
1:3

julia> convert(UnitRange, Base.OneTo(5))
1:5

julia> methodswith(AbstractUnitRange)
[1] getindex(r::AbstractUnitRange, s::AbstractUnitRange{var"#s91"} where var"#s91"<:Integer) in Base at range.jl:695
[2] getindex(r::AbstractUnitRange, s::StepRange{var"#s91",S} where S where var"#s91"<:Integer) in Base at range.jl:709
[3] getindex(A::SparseArrays.AbstractSparseMatrixCSC{Tv,Ti} where Ti<:Integer, I::AbstractUnitRange) where Tv in SparseArrays at C:\Users\kittisopikulm\AppData\Local\Programs\Julia 1.5.0\share\julia\stdlib\v1.5\SparseArrays\src\sparsevector.jl:626
[4] getindex(x::SparseArrays.AbstractSparseMatrixCSC, I::AbstractUnitRange, j::Integer) in SparseArrays at C:\Users\kittisopikulm\AppData\Local\Programs\Julia 1.5.0\share\julia\stdlib\v1.5\SparseArrays\src\sparsevector.jl:531
[5] getindex(x::SparseArrays.AbstractSparseArray{Tv,Ti,1}, I::AbstractUnitRange) where {Tv, Ti} in SparseArrays at C:\Users\kittisopikulm\AppData\Local\Programs\Julia 1.5.0\share\julia\stdlib\v1.5\SparseArrays\src\sparsevector.jl:784
[6] checkindex(::Type{Bool}, inds::AbstractUnitRange, i::Real) in Base at abstractarray.jl:563
[7] checkindex(::Type{Bool}, inds::AbstractUnitRange, ::Colon) in Base at abstractarray.jl:564
[8] checkindex(::Type{Bool}, inds::AbstractUnitRange, ::Base.Slice) in Base at abstractarray.jl:565
[9] checkindex(::Type{Bool}, inds::AbstractUnitRange, r::AbstractRange) in Base at abstractarray.jl:566
[10] checkindex(::Type{Bool}, indx::AbstractUnitRange, I::Base.LogicalIndex) in Base at multidimensional.jl:695
[11] checkindex(::Type{Bool}, indx::AbstractUnitRange, I::AbstractArray{Bool,1}) in Base at abstractarray.jl:570
[12] checkindex(::Type{Bool}, indx::AbstractUnitRange, I::AbstractArray{Bool,N} where N) in Base at abstractarray.jl:571
[13] checkindex(::Type{Bool}, inds::AbstractUnitRange, I::AbstractArray) in Base at abstractarray.jl:572
[14] checkindex(B::Type{Bool}, inds::AbstractUnitRange, i::StaticArrays.StaticIndexing{T}) where T in StaticArrays at C:\Users\kittisopikulm\.julia\packages\StaticArrays\LJQEe\src\indexing.jl:368
[15] checkindex(::Type{Bool}, inds::AbstractUnitRange, i) in Base at abstractarray.jl:561
[16] findfirst(p::Union{Base.Fix2{typeof(==),T}, Base.Fix2{typeof(isequal),T}}, r::AbstractUnitRange) where T<:Integer in Base at array.jl:1867
[17] isempty(r::AbstractUnitRange) in Base at range.jl:503
[18] issorted(r::AbstractUnitRange) in Base at range.jl:1011
[19] length(r::AbstractUnitRange) in Base at range.jl:545
[20] maximum(r::AbstractUnitRange) in Base at range.jl:602
[21] minimum(r::AbstractUnitRange) in Base at range.jl:601
[22] sort(r::AbstractUnitRange) in Base at range.jl:1014
[23] sort!(r::AbstractUnitRange) in Base at range.jl:1015
[24] sortperm(r::AbstractUnitRange) in Base at range.jl:1019
[25] step(r::AbstractUnitRange{T}) where T in Base at range.jl:528
[26] view(r1::AbstractUnitRange, r2::AbstractUnitRange{var"#s91"} where var"#s91"<:Integer) in Base at subarray.jl:167
[27] view(r1::AbstractUnitRange, r2::StepRange{var"#s91",S} where S where var"#s91"<:Integer) in Base at subarray.jl:171

@vtjnash vtjnash added this to the 1.8 milestone Apr 19, 2021
@oschulz
Copy link
Contributor

oschulz commented Apr 25, 2021

Just an idea: What about allowing one:n as a convenient shortcut for Base.OneTo(n)? Would be short and very readable.

@oschulz
Copy link
Contributor

oschulz commented Apr 25, 2021

There's currently no method that matches one:(n::Integer)

julia> one:5
ERROR: MethodError: no method matching (::Colon)(::typeof(one), ::Int64)

So defining (:)(::typeof(one), n::Integer) would be non-breaking.

Just a quick test:

julia> import Base.:

julia> (:)(::typeof(one), n::Integer) = Base.OneTo(n)
(::Colon) (generic function with 16 methods)

julia> one:5
Base.OneTo(5)

@mkitti
Copy link
Contributor Author

mkitti commented Apr 25, 2021

Just an idea: What about allowing one:n as a convenient shortcut for Base.OneTo(n)? Would be short and very readable.

That's an interesting thought and could be easily implemented:

julia> (::Colon)(::typeof(one), x::Integer) = Base.OneTo(x)

julia> one:5
Base.OneTo(5)

I would expect the discussion of that to go along the lines of #39242 . It is also distinct from the objectives of this PR. I encourage you to submit a new PR or issue to propose this.

@StefanKarpinski
Copy link
Sponsor Member

Looking at these again, I don't really have any objection: they all fall out of the single assumption that the default start value is one, which seems easy enough to explain and understand.

@mkitti
Copy link
Contributor Author

mkitti commented Aug 6, 2021

Looking at these again, I don't really have any objection: they all fall out of the single assumption that the default start value is one, which seems easy enough to explain and understand.

The other assumption is that step is one.

Note that this PR does not allow for a Pythonic range(5).

@StefanKarpinski
Copy link
Sponsor Member

Note that this PR does not allow for a Pythonic range(5).

Right. Last time around there were enough of these range PRs that it was a bit hard to sort out what was adding what. Now that we've made the other changes, it's clearer to me at least that all this does is make start = 1 the default and allow combining that default with stop and length keywords.

@mkitti
Copy link
Contributor Author

mkitti commented Aug 6, 2021

It's worth to consider all the places in julia as well as in the ecosystem where method arguments are restricted to UnitRange, e.g.,

julia> methodswith(typeof(1:3))
[1] getindex(A::Array, I::UnitRange{Int64}) in Base at array.jl:805
...

Changing the return type of range might not be ideal from this perspective.

The first method in that list was generalized to AbstractUnitRange by #39896 . I'll create a few more pull requests to address the remaining ones.

@oschulz
Copy link
Contributor

oschulz commented Aug 7, 2021

I'm not sure if there's already an issue on that - the range-from-one and range-from-zero could be done very elegantly if we had precision-independent representations for one and zero in the language, with separate types for one and zero (along the lines of https://github.com/perrutquist/Zeros.jl), like we have with pi and so on. It would be very useful in many contexts anyway (often we use true/false as a minimum-precision one/zero literal, but it's not very readable and can't be dispatched on).

This approach has worked nicely for constants like π and , and one and zero are even more fundamental than those two. :-)

@mkitti
Copy link
Contributor Author

mkitti commented Aug 7, 2021

I'm not sure if there's already an issue on that - the range-from-one and range-from-zero could be done very elegantly if we had precision-independent representations for one and zero in the language

I think the concept of Zeros.jl sounds great, but I'm quite unclear how range-from-one or range-from-zero might be done elegantly from that. Using types already in Base there are the method types typeof(one) and typeof(zero) as well as typeof(oneunit). Perhaps the positional or keyword start could accept those as an argument?

Anyways, modifying start behavior is beyond the scope of this PR. Please create an issue or a PR on this matter.

@mkitti
Copy link
Contributor Author

mkitti commented Aug 7, 2021

I have resolved the conflicts that were present due to other changes on the master branch. The only failing test is for buildbot/tester_freebsd64 which seems unrelated.

Also I have looked into potential issues (#41805, #41807 and #41809) with using Base.OneTo by expanding several Base methods to AbstractUnitRange from UnitRange. Most of those changes seem manageable. A simplification of this PR would be to use UnitRange rather than Base.OneTo to implement the single keyword argument range.

@oschulz
Copy link
Contributor

oschulz commented Aug 9, 2021

I think the concept of Zeros.jl sounds great, but I'm quite unclear how range-from-one or range-from-zero might be done elegantly from that.

If we had special types for one and zero, let's call them One an Zero (we should define convenience literals), then it would be quite natural that

Zero():n == Base.ZeroTo(n)
One():n == Base.OneTo(n)

(We currently don't have Base.ZeroTo - yet? - of course).

That would be even cleaner than one:n and zero:n, and having a dispatchable one and zero would open up lot's of potential in other places as well (removing the need for constructs like FillArrays.Ones, etc.

@mkitti
Copy link
Contributor Author

mkitti commented Aug 9, 2021

I think the concept of Zeros.jl sounds great, but I'm quite unclear how range-from-one or range-from-zero might be done elegantly from that.

If we had special types for one and zero, let's call them One an Zero (we should define convenience literals), then it would be quite natural that

Zero():n == Base.ZeroTo(n)
One():n == Base.OneTo(n)

(We currently don't have Base.ZeroTo - yet? - of course).

That would be even cleaner than one:n and zero:n, and having a dispatchable one and zero would open up lot's of potential in other places as well (removing the need for constructs like FillArrays.Ones, etc.

juila> import Base: one, zero

julia> (::Colon)(::typeof(zero), n) = zero(n):n

julia> (::Colon)(::typeof(one), n) = Base.OneTo(n)

julia> zero:5
0:5

julia> one:3
Base.OneTo(3)

@oschulz
Copy link
Contributor

oschulz commented Aug 9, 2021

Sure, one:n we can do right now, adding types or one and zero would obviously be a separate issue. I'm not sure we should support zero:n without adding Base.ZeroTo as well, because we'd need a breaking change later on when a Base.ZeroTo is (hopefully) added in the future.

@StefanKarpinski StefanKarpinski added the triage This should be discussed on a triage call label Aug 9, 2021
@mkitti
Copy link
Contributor Author

mkitti commented Aug 9, 2021

@oschulz , thank you for the ideas. Can we continue this conversation over in #41840 ?

@oschulz
Copy link
Contributor

oschulz commented Aug 9, 2021

Can we continue this conversation over in #41840 ?

Oh, sure!

@vtjnash vtjnash merged commit 5ff9e3a into JuliaLang:master Aug 16, 2021
@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Aug 17, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
As a single keyword arg only (single positional arg not allowed still)
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
As a single keyword arg only (single positional arg not allowed still)
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