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

Improve docs for vcat, cat, etc. #46429

Merged
merged 6 commits into from
Oct 29, 2022
Merged

Conversation

mcabbott
Copy link
Contributor

@mcabbott mcabbott commented Aug 21, 2022

This wants to:

  • Mention the fast methods for reduce(vcat, A) under reduce and under vcat, and state exactly what types they accept. As far as I know this was documented nowhere besides methods(reduce).
  • Mention stack from #43334 as an alternative where appropriate, including under eachslice and mapslices.
  • Provide better examples for vcat, hcat. They are about the same length, but try not to print out things like b = [6 7; 8 9; 10 11; 12 13; 14 15] too often, instead illustrating more uses, from simple to complex.
  • Break up the wall of text for cat(A...; dims), and avoid saying the word "stacked" there.

@ViralBShah ViralBShah added the docs This change adds or pertains to documentation label Aug 23, 2022
@mikmoore
Copy link
Contributor

mikmoore commented Aug 23, 2022

Before we canonize reduce(vcat,itr) and friends as the proper way to do these things, should we consider whether this is actually something we want to do? I've always despised these specializations. They're difficult to discover (this doc PR helps) and they don't compose or behave like you'd hope, e.g. reduce(vcat ∘ vec,itr) or mapreduce(vec,vcat,itr) will totally miss the specializations and be slow as anything.

#43334 applies in a narrow subset of cases. #46003 would apply in some others. But really, we still lack a generic function for concatenation of iterables. cat provides the analog of min but we lack an analog of minimum. I closed #45563 awhile back but I don't think stack from #43334 really met my hopes on this matter.

@mcabbott
Copy link
Contributor Author

These specialisations probably cannot be removed, so at least they should be clearly documented, including the precise signature which triggers them. If I'm not mistaken you have to look at methods(reduce) to know this, at present.

I agree they are hard to discover, and fragile. stack covers only some uses. Maybe other cases are off-topic for this PR so I fold them here:

Similar code could certainly be written for an efficient eager flatten; in some sense that would be the reduce(vcat, v_v) to match stack's reduce(hcat, v_v), and has the precedent of Iterators.flatten to describe exactly what it does. Whether it's likely to be approved I don't know.

Many other rules are possible. If I understand right, #46003 proposes a rule with a kron-like combining of the axes of inner and outer arrays:

julia> A = [rand(2,3) for _ in 1:4, _ in 1:5];

julia> awfulstack(A) == hvcat(5, permutedims(A)...)
true

julia> using TensorCast

julia> awfulstack(A) == @cast out[(i,I), (j,J)] := A[I,J][i,j]
true

julia> stack(A) == @cast out[i,j,I,J] := A[I,J][i,j]
true

julia> awfulstack(A) == @cast out[(i,I), (j,J)] := stack(A)[i,j,I,J]
true

On a uniform-size array of arrays like A, this is (like flatten) some reshape & permutedims of stack. There's also some extension to non-uniform arrays which I don't quite see.

base/reducedim.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
@mikmoore
Copy link
Contributor

Technically, removing these performance shortcuts would be non-breaking since the fallback is still operable. That said, reversion to the fallback would impact performance very negatively and for that reason I imagine we'd keep them for a long time anyway.

I expect that, if we do ever manage a more generic function, these specialization could still exist (for legacy reasons) although be replaced by forwarding to that function. So, with that in mind and with the lack of an in-progress PR to create the aforementioned generalization, I'll retract my reservations about documenting them.

@mcabbott
Copy link
Contributor Author

Besides performance there are quirks like this which would probably break things:

julia> mapreduce(identity, hcat, [[1,2]])  # never calls hcat
2-element Vector{Int64}:
 1
 2

julia> reduce(hcat, [[1,2]])
2×1 Matrix{Int64}:
 1
 2

Perhaps we could make reduce(hcat, ::AbstractVector{<:AbstractVector}) call stack now, but there's not much gain, and maybe there are other weird edge cases.

base/abstractarray.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
@mcabbott
Copy link
Contributor Author

Bump... pinging @ViralBShah as he liked it in August?

@ViralBShah
Copy link
Member

ViralBShah commented Oct 29, 2022

Thanks. I will come around to this soon. In general, I feel anything that is an improvement and is correct in a docs PR should be merged.

@ViralBShah ViralBShah merged commit 27201e5 into JuliaLang:master Oct 29, 2022
@mcabbott mcabbott deleted the cat_doc branch November 1, 2022 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants