optimize reduce(vcat, vector_of_matrices)

[rfourquet]

This is the idiomatic way to call vcat on a vector of matrices,
at least when performance matters (as opposed to
vcat(vector_of_matrices...)).
Let's optimize this case, like it's done in Base for Vector
and Matrix.
Similarly for hcat.

This is an alternative solution to the special implementation vcat(A::Vector{<: MatrixElem}) which was removed in #409.

[fieker]

On Mon, Oct 14, 2019 at 04:53:46AM -0700, Rafael Fourquet wrote: This is the idiomatic way to call vcat on a vector of matrices, at least when performance matters (as opposed to `vcat(vector_of_matrices...)`). Let's optimize this case, like it's done in `Base` for `Vector` and `Matrix`. Similarly for `hcat`.

How is one supposed to figure out that reduce is the "best" and "most natural" way of doing things? Sometimes, their choices are strange...

…

This is an alternative solution to the special implementation `vcat(A::Vector{<: MatrixElem})` which was removed in #409. You can view, comment on, or merge this pull request online at: #496 -- Commit Summary -- * optimize reduce(vcat, vector_of_matrices) -- File Changes -- M src/generic/Matrix.jl (5) M test/generic/Matrix-test.jl (3) -- Patch Links -- https://github.com/Nemocas/AbstractAlgebra.jl/pull/496.patch https://github.com/Nemocas/AbstractAlgebra.jl/pull/496.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #496

[rfourquet]

How is one supposed to figure out that reduce is the "best" and "most natural" way of doing things?

I consider the situation to be subotimal in Base, and I hope the status quo will evolve for the better.

When you have a binary operation, like vcat, and you want to apply it to a collection one element at a time, reduce is the goto solution. But I would not have thought about it for vcat because it seems obvious it will be badly inefficient, by allocating a new result matrix at each iteration. However, they decided it will still be the current "blessed" way to do that, and they optimized this particular case to not allocate like crazy. So I find it unsatisfying because the non-naïve user won't think about this solution unless she knows it's optimized. Moreover, it works only for AbstractVector's of matrices, not for general iterators...
But, for the time being, this is the solution they chose, and it's easy to support the same in our case.

[wbhart]

Does anyone have an objection to merging this? @thofma @fieker

[thofma]

Have no clue what this is doing, so no objection from me :)

[fieker]

On Wed, Oct 23, 2019 at 09:17:08AM -0700, wbhart wrote: Does anyone have an objection to merging this? @thofma @fieker

Does this break anyhting that is currently working? If not go ahead. If it breaks s.th., then no

…

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #496 (comment)

[rfourquet]

Have no clue what this is doing,

This is making reduce(vcat, [M1, M2, M3]) "fast", where M1, M2, M3 are matrices. I.e. it doesn't allocate a new matrix for each intermediate step of reduce.

Does this break anyhting that is currently working?

I don't see what it could break. It seems to be an "optimization only" change.

[fieker]

On Thu, Oct 24, 2019 at 09:32:21AM +0000, Rafael Fourquet wrote: > Have no clue what this is doing, This is making `reduce(vcat, [M1, M2, M3])` "fast", where `M1, M2, M3` are matrices. I.e. it doesn't allocate a new matrix for each intermediate step of `reduce`. > Does this break anyhting that is currently working? I don't see what it could break. It seems to be an "optimization only" change.

... then go ahead I think we're using a different signature to do the same and to be fast

…

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #496 (comment)