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

Add empty for AbstractArrays and their types #33482

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add empty for AbstractArrays and their types #33482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions base/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,9 +672,10 @@ similar(::Type{T}, shape::Tuple{Union{Integer, OneTo}, Vararg{Union{Integer, One
similar(::Type{T}, dims::Dims) where {T<:AbstractArray} = T(undef, dims)

"""
empty(v::AbstractVector, [eltype])
empty(a::AbstractArray, [eltype])
empty(a::Type{AbstractArray}, [eltype])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
empty(a::Type{AbstractArray}, [eltype])
empty(a::Type{<:AbstractArray}, [eltype])


Create an empty vector similar to `v`, optionally changing the `eltype`.
Create an empty array similar to `a`, optionally changing the `eltype`.

# Examples

Expand All @@ -687,6 +688,10 @@ julia> empty([1.0, 2.0, 3.0], String)
```
"""
empty(a::AbstractVector{T}, ::Type{U}=T) where {T,U} = Vector{U}()
Copy link
Member

Choose a reason for hiding this comment

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

change this method also then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left that in for inlining purposes due to all the existing code depending on it. If that is not necessary, then I'll change it as well.

function empty(a::Type{<:AbstractArray{T,N}}, ::Type{U}=T) where {T,N,U}
convert(AbstractArray{U}, a(undef, Tuple(Iterators.repeated(0,N))))
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I'm not sure T(undef, dims) is a required part of the AbstractArray API. The operation of creating a new array from a type but with a different element type could potentially be handled by similar (see #20815).

Also this should be faster:

Suggested change
convert(AbstractArray{U}, a(undef, Tuple(Iterators.repeated(0,N))))
convert(AbstractArray{U}, a(undef, ntuple(_ -> 0, N)))

BTW, better use the short function syntax as for other definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the ntuple correction! I actually found that function some days ago and was gonna modify the PR accordingly but you were faster. :)

About the T(undef, dims), yeah, sorry; that was stupid since similar has the dims argument. I completely forgot about it.
I also love your referenced PR; just today I ran into a case where it would have been useful.

BTW, better use the short function syntax as for other definitions.

Do you mean I should always use empty(x) = ...? I didn't use it as the functions were getting too wide (over 92 columns) and I thought the short syntax should be avoided when the function spans multiple lines.

Copy link
Member

Choose a reason for hiding this comment

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

About the T(undef, dims), yeah, sorry; that was stupid since similar has the dims argument. I completely forgot about it.
I also love your referenced PR; just today I ran into a case where it would have been useful.

As I noted, the problem with similar is that it requires an instance, not a type.

Do you mean I should always use empty(x) = ...? I didn't use it as the functions were getting too wide (over 92 columns) and I thought the short syntax should be avoided when the function spans multiple lines.

Yes it's fine to break after = (see examples in this file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I noted, the problem with similar is that it requires an instance, not a type.

That was the problem with it, I remember... So first we will have to keep the constructor by type, if I'm seeing this correctly? Until #20815 is merged.

Yes it's fine to break after = (see examples in this file).

I was thinking more in an aesthetic/readability sense. Only because it has been done in the past does not imply it has to be done again. But sure, I will change it!

Copy link
Member

Choose a reason for hiding this comment

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

Actually I'm not even sure calling the constructor like that is guaranteed to work. Somebody else will have to confirm.

end
empty(a::AbstractArray{T}, ::Type{U}=T) where {T,U} = empty(typeof(a), U)

# like empty, but should return a mutable collection, a Vector by default
emptymutable(a::AbstractVector{T}, ::Type{U}=T) where {T,U} = Vector{U}()
Expand Down
12 changes: 12 additions & 0 deletions test/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,11 +892,23 @@ end
v = [1, 2, 3]
v2 = empty(v)
v3 = empty(v, Float64)
v4 = empty(typeof(v))
v5 = empty(typeof(v), Float64)
@test !isempty(v)
empty!(v)
@test isempty(v)
@test isempty(v2::Vector{Int})
@test isempty(v3::Vector{Float64})
@test isempty(v4::Vector{Int})
@test isempty(v5::Vector{Float64})

m = [1 2; 3 4]
# These call `empty(typeof(m))`, so we don't test that case.
m2 = empty(m)
m3 = empty(m, Float64)
@test !isempty(m)
@test isempty(m2::Matrix{Int})
@test isempty(m3::Matrix{Float64})
end

@testset "CartesianIndices" begin
Expand Down