Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[RFC] Update ICA interface and implementation #122

Closed
wants to merge 8 commits into from

Conversation

baggepinnen
Copy link

@baggepinnen baggepinnen commented Jul 7, 2020

This PR is aimed at starting a discussion around the interface and implementation of ICA.

The current implementation uses symbols for dispatch which means that one can not add new algorithms without modifying the source. It also uses an awkward "function to get a function" do dispatch on the derivative type.

This PR:

  • changes the algorithm dispatch to using an AbstractICAAlg
  • Dispatches directly on the ICADeriv type and gets rid of the icaderiv middlehand.
  • Refactors the loop out into self contained functions. This is arguably a little bit uglier than the previous approach, but it has a huge benefit: one can easily modify these new functions to exploit SIMD for 5x+ greater performance.
    Some performance benchmarks on the computationally heavy part of the algorithm:
130.192 μs (1 allocation: 32 bytes) # Baseline
56.411 μs (1 allocation: 32 bytes)  # @avx
52.914 μs (1 allocation: 32 bytes)  # SLEEFPirates master
21.719 μs (1 allocation: 32 bytes)  # SLEEFPirates.tanh_fast

The factor of 6x is too big to ignore for many scenarios, but does require a dependency on LoopVectorization. Consider the two functions below, which in this PR makes the heavy lifting

function update_UE!(f::Tanh{T}, U::AbstractMatrix{T}, E1::AbstractVector{T}) where T
    n,k = size(U)
    _s = zero(T)
    a = f.a
    @inbounds for j = 1:k
        for i = 1:n 
            t = tanh(a * U[i,j])
            U[i,j] = t
            _s += a * (1 - t^2)
        end
        E1[j] = _s / n
    end
end
function update_UE!(f::Tanh{T}, U::AbstractMatrix{T}, E1::AbstractVector{T}) where T
    n,k = size(U)
    _s = zero(T)
    a = f.a
    @inbounds for j = 1:k
        @avx for i = 1:n 
            t = SLEEFPirates.tanh_fast(a * U[i,j])
            U[i,j] = t
            _s += a * (1 - t^2)
        end
        E1[j] = _s / n
    end
end

the second version using LoopVectorization and SLEEFPirates is 6x faster on my machine. A user wanting to make this happen can simply implement a new ICADeriv type Tanhfast and overload update_UE!, without MultivariateStats taking a dependency on LoopVectorization.jl

This is obviously a breaking change, but I hope people will find it for the better.

wildart added a commit to wildart/MultivariateStats.jl that referenced this pull request Jan 20, 2022
@wildart
Copy link
Collaborator

wildart commented Jan 20, 2022

Merged in #174.

@wildart wildart closed this Jan 20, 2022
wildart added a commit that referenced this pull request Jan 23, 2022
* Added docstrings, and updated `ICAGDeriv` as in #122
* added ICA docs
* updated ICA test
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants