Coverage remained the same when pulling d4344f5 on alimuldal:fastica_svd_decorr into b3a9da9 on scikit-learn:master.

Coverage remained the same when pulling d4344f5 on alimuldal:fastica_svd_decorr into b3a9da9 on scikit-learn:master.

@alimuldal @GaelVaroquaux

I've run some basic performance benchmarks and I don't think the changes will incur meaningful costs.

proposed changes with both options:

0-15-git master:

Also see:
https://gist.github.com/dengemann/9091092

I'll soon check whether the new options improve estimation in a few applications of mine. For the PR this should not be relevant though.

Here's another performance plot with more realistic parameters (50 components, 250k samples, 250 features). We now clearly see the difference.

I think that's fair enough given that one can choose between both options.

Just want to pull your attention to the following observation nilearn/nilearn#152, discussing a similar case, but in which we have opted to change from the scipy.linalg.svd with the option full_matrices = False to a symmetric eigenvalue decomposition (the former is problematic if numpy is compiled against lapack_lite)

Base on the fact that numpy compiled with lapack_lite will generate
problems in the case of the SVD, I suggest that:

• We remove the option svd_decorr: too many options end up being
  confusing for the user
• We default to using SVD
• If numpy is compiled with lapack_lite, we raise a meaningful warning,
  and use eigsh

To check if numpy is compiled with lapack_lite, you can try to import
numpy.core._dotblas. If the import fails, numpy is compiled with
lapack_lite.

@GaelVaroquaux +1 for your proposal. However, to fully embrace not adding a new option I first need to see some down-stream improvements in analyses. On the other hand, we can still add the option later if it turns out relevant.

However, to fully embrace not adding a new option I first need to see
some down-stream improvements in analyses.

For the original poster, it lead to an improvement.

For the original poster, it lead to an improvement.

Then let's add it for now. Probably it's a good bet and I'll be happy too. If not I'll report + propose changes.

@GaelVaroquaux @agramfort
Btw. 'svd_deorr=True' as internal default would not affect MNE tests + examples.
Good to go from my side.

what is the status on this?

is the improvement significant? what's the gain?

I see that FastICA now had a whiten_solver that has a SVD option, does this mean this PR is not needed anymore? And what does it mean about the issue?

yes.

thanks @haiatn for chasing outdated PRs

and thanks @alimuldal for your effort here.