Skip to content

Allow collect to return non-Array types #58513

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

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

Allow collect to return non-Array types #58513

wants to merge 1 commit into from

Conversation

nalimilan
Copy link
Member

@nalimilan nalimilan commented May 23, 2025

As discussed at #36448 and triaged at #50051 (comment). This is already what some packages such as OffsetArrays, StaticArrays and CategoricalArrays do.

See also #16029, #36448.

As discussed at #36448. This is already what some packages such as StaticArrays and CategoricalArrays do. Without this, `collect` is redundant with `Array(itr)` or `Array{T}(itr)`.
@nalimilan nalimilan added docs This change adds or pertains to documentation needs decision A decision on this change is needed arrays [a, r, r, a, y, s] labels May 23, 2025
@adienes
Copy link
Member

adienes commented May 23, 2025

at the very least it should specify AbstractArray I think. otherwise "array" (lowercase) is not very well specified

@jakobnissen
Copy link
Member

jakobnissen commented May 24, 2025

I object to this. It makes collect more confusing and harder to reason about.
If anything, we should push the opposite direction. Collect was originally intended to always return Vector: #47777

Edit: Although triage disagrees with me... So maybe the docstring should follow triage: #50051 (comment)

@adienes
Copy link
Member

adienes commented May 24, 2025

I think also collect should demand that the output HasShape. technically an AbstractArray could be infinite; I do not thing something "collected" can be infinite.

(does this also close #16029, #36106)

@nsajko
Copy link
Contributor

nsajko commented May 25, 2025

Cross-posting from the linked issue:

FTR I've put up a package called CollectAs, currently living here and not yet registered. The collect_as function provided by the package generalizes collect by taking two arguments, where the first argument is not, like with collect, the element type of the return type, rather it's just the return type. This means that packages like StaticArrays.jl or OffsetArrays.jl, or most collections actually, will soon be able to choose to take a (strong or weak) dependency on CollectAs.jl and add methods with their own types as the return type.

That seems like it decreases the desirability of making collect return anything other than Array.

@nalimilan
Copy link
Member Author

If anything, we should push the opposite direction. Collect was originally intended to always return Vector: #47777

Edit: Although triage disagrees with me... So maybe the docstring should follow triage: #50051 (comment)

@jakobnissen Yes, the idea is that if that operation is supposed to return an array, writing Array(itr)/Vector(itr) or Array{T}(itr)/Vector{T}(itr) is more explicit. If this PR is merged, it would make sense to also define these methods. OTOH, in many cases people use collect just like they use similar, asking for the most appropriate mutable array type for the data at hand, and they don't necessarily need an Array.

at the very least it should specify AbstractArray I think. otherwise "array" (lowercase) is not very well specified

@adienes It's true that it's not super precise, but this is how the docstring for similar is written too. Not sure whether that's intended (nicer to read) or just written that way for historical reasons.

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] docs This change adds or pertains to documentation needs decision A decision on this change is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants