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

Add empty for AbstractArrays and their types #33482

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

Conversation

janEbert
Copy link
Contributor

@janEbert janEbert commented Oct 6, 2019

empty now works on all AbstractArrays and the corresponding call
using Type{<:AbstractArray}.
The method specialized on AbstractVector remains for inlining
purposes. I'm not sure whether that is actually necessary but I'm sure you can tell. (And maybe also explain how to test for that myself. That would be amazing!)

`empty` now works on all `AbstractArray`s and the corresponding call
using `Type{<:AbstractArray}`.
The method specialized on `AbstractVector` remains for inlining
purposes.
@rfourquet
Copy link
Member

The motivation to make empty work on types (instead of instances) is not clear here. Also, empty is defined for other kind of containers, so the same would have to be done for those too.

@janEbert
Copy link
Contributor Author

janEbert commented Oct 6, 2019

Assume you have a Vector{Vector{Int}} of sequence data. You cannot construct an empty element for it using eltype.
Sorry, not looking for other methods of empty was a silly oversight of me. I will fix it.

@rfourquet
Copy link
Member

You cannot construct an empty element for it using eltype.

At least In this particular case you can do eltype(Vector{Vector{Int}})()

Sorry, not looking for other methods of empty was a silly oversight of me. I will fix it.

It's a good starting point, and I would wait for more feedback before spending time on updating this PR to other methods.

@janEbert
Copy link
Contributor Author

janEbert commented Oct 6, 2019

Hehe, you are right. I was just confused when it didn't work and wanted to patch the hole since most methods usually take types rather than instances (or both).

It's a good starting point, and I would wait for more feedback before spending time on updating this PR to other methods.

Thanks for saying that, I will not continue for now, then. However, I had already started and found that writing every method for instances and types was not good design.

If this seems worthwhile, maybe having a generic

#= @inline ? =# empty(a, types...) = empty(typeof(a), types...)

just like for eltype would be a good idea.

@@ -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.

@janEbert
Copy link
Contributor Author

janEbert commented Oct 13, 2019

I also encountered another use case. Pre-allocating a Vector of Arrays of high dimensionality requires you to write an array comprehension with the awkward Array{Int, 25}(undef, Tuple(Iterators.repeated(0, 25))) constructor.
Even for saner dimensionalities, this quickly leads to unreadable code where empty(Array{Int, 25}) is much quicker to process.

@kshyatt kshyatt added the arrays [a, r, r, a, y, s] label Oct 15, 2019
@mbauman
Copy link
Member

mbauman commented Oct 15, 2019

I have a few questions for which I don't have solid answers:

  • Does it make sense for the other methods of empty (the non-array ones) to allow types, too?
  • Should we have a fallback for empty(x) = empty(typeof(x)) a la eltype? What would the default empty(::Type) be? Maybe just have this fallback for ::AbstractArray?
  • Are there any cases where you need the object itself to determine what its empty looks like (and the type doesn't have enough information)?

@nalimilan
Copy link
Member

* Does it make sense for the other methods of `empty` (the non-array ones) to allow types, too?

I'd say yes.

* Should we have a fallback for `empty(x) = empty(typeof(x))` a la `eltype`?  What would the default `empty(::Type)` be?  Maybe just have this fallback for `::AbstractArray`?

Yes, makes sense.

* Are there any cases where you need the object itself to determine what its `empty` looks like (and the type doesn't have enough information)?

Hopefully not. Actually that's a question we should also try to address for similar, as having a version of it taking a type rather than an instance would be useful in other situations (#20815).

@@ -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])

@@ -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}()
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.

@StefanKarpinski
Copy link
Member

It's usually quite a bad idea to introduce methods that work on both types and instances of types since types are values in Julia so this tends to produce an inherent ambiguity. However, in this case the empty function only makes sense for collections and types are not collections, so it seems safe and unambiguous to me:

  • if the argument is a type, try to create an empty instance of that type
  • if the argument is not a type, type to create an empty instance of the value's type

@janEbert
Copy link
Contributor Author

janEbert commented Nov 1, 2019

Hey Stefan, so you are in favour of something like
empty(a, types...) = empty(typeof(a), types...)?
I'm sorry this hasn't moved along at all, I'm a bit busy this month but once I find the time, I'll get back to this taking all the feedback into account! Thank you all.

@StefanKarpinski
Copy link
Member

Sure, I think that would be fine.

@fingolfin
Copy link
Contributor

@janEbert Sorry for neglecting this PR for 4 years. It now has conflicts. If you are still interested and would be willing to update it, I think it could be merged soon?

@janEbert
Copy link
Contributor Author

Hi Max, sorry from my side; after all I promised to add stuff to the PR but never did. I haven't used Julia deeply in a while but if you are fine with waiting a bit more for me to catch up again, I'll gladly update the PR according to all the 4 amazing years of development. ;)

@fingolfin
Copy link
Contributor

@janEbert not your fault. thank you for being willing to pick this up again -- and sure, take your time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants