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

hcat is incredibly slow for more than two inputs datasets #38

Open
kahaaga opened this issue Nov 16, 2024 · 5 comments
Open

hcat is incredibly slow for more than two inputs datasets #38

kahaaga opened this issue Nov 16, 2024 · 5 comments

Comments

@kahaaga
Copy link
Member

kahaaga commented Nov 16, 2024

Describe the problem

After https://github.com/JuliaDynamics/StateSpaceSets.jl/pull/32/files was merged, hcat is incredibly slow when horizontally concatenating. This is true for both Vector{<:Real} with StateSpaceSets. The operation allocates excessively and is very slow.

The hcat I had implemented before the PR was pretty fast, so I think we should just bring that back in new wrapping, so that it works for any container type.

Minimal Working Example

Illustrative example:

julia> using StateSpaceSets
julia> x = rand(10); y = rand(10, 2) |> StateSpaceSet; z = rand(10) |> StateSpaceSet;
julia> hcat(x, y, z); hcat(x, Matrix(x), Matrix(z));
Julia> using BenchmarkTools
julia> @btime hcat($x, $y, $z);
  18.833 μs (341 allocations: 10.98 KiB)
julia> @btime hcat($x, $(Matrix(y)), $(Matrix(z)));
  28.991 ns (2 allocations: 416 bytes)

hcat with state space sets is 650 times slower than hcat on matrices, which of course will have huge impacts in any code that relies on horizontal concatenation of state space sets (e.g. in Associations.jl).

Package versions

I'm using StateSpaceSets 2.3.0, but this is likely the case on all version following the PR linked above.

@kahaaga
Copy link
Member Author

kahaaga commented Nov 16, 2024

Update: just a simple wrapper that first converts the state space sets to a matrix, then converts back using a user-provided container type is already 9.5 times as fast and allocates 3 times less.

function merge_vector_or_statespacesets(x::VectorOrStateSpaceSet...; container = SVector)
    StateSpaceSet(hcat((xᵢ isa AbstractStateSpaceSet ? Matrix(xᵢ) : xᵢ for xᵢ in x)...); container)
end
julia> @btime hcat($x, $y, $z);
  19.041 μs (341 allocations: 10.98 KiB)
julia> @btime merge_vector_or_statespacesets($x, $y, $z);
  2.620 μs (53 allocations: 3.22 KiB)  

@kahaaga
Copy link
Member Author

kahaaga commented Nov 16, 2024

But this can be made much faster by bringing back the old approach of inferring the dimension up-front, pre-allocating an array to fill into, and then converting to the desired state space set container type at the end.

@Datseris
Copy link
Member

Datseris commented Nov 16, 2024

Sure, of course you can bring back the new approach. During v2 it took a lot of time to make the code be correct, so performance was not within my time budget.

@kahaaga
Copy link
Member Author

kahaaga commented Nov 16, 2024

Sure, of course you can bring back the new approach. During v2 it took a lot of time to make the code be correct, so performance was not within my time budget.

Makes sense! I'll just use a convenience function upstream until this is fixed here (read: until I have time to reintroduce the old approach in a PR here).

@Datseris
Copy link
Member

I'll just use a convenience function upstream

I don't understand. If you have this function why not put it here directly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants