-
-
Notifications
You must be signed in to change notification settings - Fork 26.2k
[WIP] No longer use SVD for fastica #11860
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
Conversation
It might make sense to decide how to solve depending on p / n, right? |
Yes this is correct. But I think that the region boundary is hard to perfectly determine. There are actually quite a lot of calls to |
It's possible to at least roughly determine a good cut-off for the heuristic, while saying "people usually use this with n>>p" is a hard to support statement and we'd do a disservice to everybody using it in a different setting, even if that's a small group. |
Ok, I have pushed a new commit where the heuristic is simply PS: Just figured out that what I called |
After discussing with @agramfort , I have rather added an option to choose different solvers for svd. Indeed, Here is a simple example to convince oneself that it accelerates import numpy as np
from sklearn.decomposition import fastica
p, n = 100, 50000 # Typical EEG size problem
n_components = 10
S = np.random.laplace(size=(p, n)) # Generate sources
A = np.random.randn(p, p) # Generate mixing matrix
X = np.dot(A, S).T # Generate signals
%timeit fastica(X, n_components=n_components, svd_solver='svd')
1.02 s ± 55.4 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
%timeit fastica(X, n_components=n_components, svd_solver='eigh')
191 ms ± 68.7 ms per loop (mean ± std. dev. of 7 runs, 1 loop each) |
@@ -203,6 +203,11 @@ def my_g(x): | |||
Initial un-mixing array of dimension (n.comp,n.comp). | |||
If None (default) then an array of normal r.v.'s is used. | |||
|
|||
svd_solver : str, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would call this whitening_solver
u, d, _ = linalg.svd(X, full_matrices=False) | ||
|
||
del _ | ||
if svd_solver == 'eigh' and n < p: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with n < p you change the behavior of existing code. Does it affect any test?
_, _, sources_fun = fastica(m.T, fun=nl, algorithm=algo, random_state=0, | ||
svd_solver='eigh') | ||
ica = FastICA(fun=nl, algorithm=algo, random_state=0, | ||
svd_solver='eigh') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the svd method still tested?
you should loop over solver options
Continuing in #22527 if anyone is interested! |
In most use cases, ICA is performed on arrays with
p << n
. In this case, usinglinalg.svd
can be order of magnitude slower than just computing the eigen decompostion ofX.dot(X.T)
:And then, SVD becomes the bottleneck of ICA!
Now, this PR breaks one test because of numerical issues (where the problem is degenerate), and I don't really know how to fix this. I do not think that this problem is likely to happen on any real case application though.
(Note that does not slow things down when
p
andn
are similar, and is only slower whenp
is much larger thann
. This hardly ever occurs in practice.)