MRG, FIX: Test round-trip, fix EEGLAB ICA

[larsoner]

Closes #8325
Closes #8308
Closes #7657

[larsoner]

On this branch I get the same plots as in #8308:

@christian-oreilly @marcpabst can you try it on your datasets to see if it makes sense? I was able to unify the code so that no branching is required, which is nice.

[larsoner]

Okay this one is ready for review @agramfort @hoechenberger . We should wait to hear from @christian-oreilly and @marcpabst before merge, though

[hoechenberger]

Added a question, otherwise LGTM.
We need to do the same thing for n_components, for sake of consistency.

[larsoner]

Argh, I went to update the documentation, now I'm confused why we need n_components, n_pca_components, and max_pca_components. I thought we'd only need two (n_pca_components and max_pca_components, and that max_pca_components determined signal reconstruction and n_pca_components was what was passed to the ICA algorithm, but this is not at all what our docs say. I'll revert my doc changes (other than the refs) and hopefully we can fix it later.

[larsoner]

@hoechenberger I'm beginning to wonder if we need max_pca_componetns to be float. Really it should be enough to allow n_components to be a float I think. The instability of the inversion comes from dividing by np.sqrt(self.pca_explained_variance_[:self.n_components_]). So if n_components is set appropriately, it seems like there shouldn't be a problem.

[hoechenberger]

@larsoner

@hoechenberger I'm beginning to wonder if we need max_pca_componetns to be float. Really it should be enough to allow n_components to be a float I think. The instability of the inversion comes from dividing by np.sqrt(self.pca_explained_variance_[:self.n_components_]). So if n_components is set appropriately, it seems like there shouldn't be a problem.

All max_pca_components are submitted to ICA for fitting; after fitting, only n_componentsICA components are retained.

If it is like you say and we can get rid of the instability caused by choosing inappropriate max_pca_components and produce sane results, then things should be okay… Still I'm a little worried. Also, max_pca_components being a float could be just another way to limit how many components to fit ICA on – would be consistent with n_components and n_pca_components

[hoechenberger]

Argh, I went to update the documentation, now I'm confused why we need n_components, n_pca_components, and max_pca_components. I thought we'd only need two (n_pca_components and max_pca_components, and that max_pca_components determined signal reconstruction and n_pca_components was what was passed to the ICA algorithm, but this is not at all what our docs say. I'll revert my doc changes (other than the refs) and hopefully we can fix it later.

Yet another example of our API being way too complicated. We've had so many issues with this…

[larsoner]

Okay let me see if I can write the docs properly

[hoechenberger]

Maybe to clarify, my understanding is:

• max_pca_components: PCA components to keep
• n_components: number of ICA components that are retained after fitting ICA on data with dimensionality reduced to max_pca_components --> the docs say otherwise, so either the docs are wrong or I made a mistake reading the sources
• n_pca_components: number of components (ICA + PCA!!!!) to use for signal reconstruction -> maybe this kwarg is just poorly named

[hoechenberger]

(edited my above post a couple times)

[larsoner]

n_components: number of ICA components that are retained after fitting ICA on data with dimensionality reduced to max_pca_components

The code has this:

        sel = slice(0, self.n_components_)
        if self.method == 'fastica':
            from sklearn.decomposition import FastICA
            ica = FastICA(whiten=False, random_state=random_state,
                          **self.fit_params)
            ica.fit(data_transformed[:, sel])

So I think the docs are correct -- ICA is fitted on the first n_components of the PCA.

n_pca_components isn't even used until apply so it only has to do with the apply step. I'll add this to the docs.

[larsoner]

Now I wonder if using n_components as float would have been enough and max_pca_components as float is overkill... I'll push a commit that shows setting n_components to a float also gives the behavior we want, even with max_pca_components=n_chan

[hoechenberger]

So I think the docs are correct -- ICA is fitted on the first n_components of the PCA.

Oh ok, I got this mixed up then.

[hoechenberger]

LGTM once CI is green

[hoechenberger]

Thanks @larsoner! I'll want to try this on some of our data now before we move forward with the API redesign, ok?