-
Notifications
You must be signed in to change notification settings - Fork 86
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
Refactor PCA and docs #163
Conversation
Hi. My name is Elias Carvalho. struct PCA{T<:Real} <: LinearDimensionalityReduction
mean::AbstractVector{T}
proj::AbstractMatrix{T}
prinvars::AbstractVector{T}
tprinvar::T
tvar::T
end To that: struct PCA{T<:Real, V<:AbstractVector{T}, M<:AbstractMatrix{T}} <: LinearDimensionalityReduction
mean::V
proj::M
prinvars::V
tprinvar::T
tvar::T
end The idea is that fields with Abstract types in structs arr not good for performance, as presented here in documentation: https://docs.julialang.org/en/v1/manual/performance-tips/#Avoid-fields-with-abstract-type |
Could you provide an example where the current type will affect performance? I guess, if you need an array type of a |
This is not a general usage of the MultivariateStats.jl/src/pca.jl Line 123 in c0f74de
So, if we test MM performance as follows struct S1{T}
a::AbstractMatrix{T}
end
struct S2{T, M<:AbstractMatrix{T}}
a::M
end
test(t::S1{T}, v::V) where {T, V<:AbstractVector{T}} = t.a*v
test(t::S2{T, M}, v::V) where {T,M<:AbstractMatrix{T},V<:AbstractVector{T}} = t.a*v
v = collect(1.0:10.)
t1 = S1(v*v')
t2 = S2(v*v')
@benchmark test(t1,v)
@benchmark test(t2,v) The difference for small matrix 10x10 is about ~50ns
And for the larger projection matrix 100x100 - almost no difference.
I post this not to discourage, if you can contribute to the package with the above modification, your PR is always welcome. |
Towards #109