Expose number of ICA iterations as ICA.n_iter_

[hoechenberger]

What does this implement/fix?

The changes in this PR enable the user to retrieve the number of completed ICA iterations via ICA.n_iter_ after ICA.fit() has been called. Currently only works with FastICA and Picard (will always be None for Infomax)

[codecov]

Codecov Report

Merging #7328 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7328      +/-   ##
==========================================
+ Coverage   89.88%   89.92%   +0.03%     
==========================================
  Files         450      450              
  Lines       81264    81437     +173     
  Branches    12918    12942      +24     
==========================================
+ Hits        73046    73230     +184     
+ Misses       5396     5386      -10     
+ Partials     2822     2821       -1     

[hoechenberger]

Tests are failing bc picard is not installed on the CI system. I see the other tests typically omit picard – why's that?

[larsoner]

See how _skip_check_picard is used

[hoechenberger]

See how _skip_check_picard is used

Yep I did see that, what's the particular reason picard tests are omitted so often?

[larsoner]

It shouldn't be that often on CIs. Just one Travis run (minimal dependencies). But we need to see the check anywhere it will be used

[hoechenberger]

@larsoner ok thanks, should all be good now

[larsoner]

Feel free to rebase or merge now that #7329 is in, then all methods can all have n_iter_

[hoechenberger]

@larsoner Rebased and squashed commits.

[larsoner]

Looks like it might disappear during I/O round trip, can you check?

[hoechenberger]

@larsoner I don't have permission to restart CI, could you please restart the one failing Travis job?

[larsoner]

Don't worry about that run, it's failing in master and is in the process of being fixed in #7325 / #7330 / #7333.

[larsoner]

Still want to see a test that asserts that n_iter_ exists and is not None after I/O round-trip

[hoechenberger]

Still want to see a test that asserts that n_iter_ exists and is not None after I/O round-trip

Oh now I understand, thought you were referring to something CI-related when you first brought this up. You want me to add a test showing that saving to and loading from disk preserves the new attribute, right? I hadn't thought about that and I'm quite certain it does not. Need to look into this.

[hoechenberger]

@larsoner
ICA.save() raises when called on an un-fitted instance:

mne-python/mne/preprocessing/ica.py

Lines 1556 to 1557 in d9e729f

	if self.current_fit == 'unfitted':
	raise RuntimeError('No fit available. Please first fit ICA')

I therefore don't understand why _write_ica(), which is invoked by save(), checks if the attributes that are supposed to be created / existing during fitting actually exist; and if they don't, simply writes out None – I mean, this should never happen, since we'd raise if not fitted. And if one of these attributes is non-existent after a fit, there clearly must be a bug somewhere.

mne-python/mne/preprocessing/ica.py

Lines 2030 to 2033 in d9e729f

	n_samples = getattr(ica, 'n_samples_', None)
	ica_misc = {'n_samples_': (None if n_samples is None else int(n_samples)),
	'labels_': getattr(ica, 'labels_', None),
	'method': getattr(ica, 'method', None)}

[larsoner]

I therefore don't understand why _write_ica(), which is invoked by save(), checks if the attributes that are supposed to be created / existing during fitting actually exist; and if they don't, simply writes out None – I mean, this should never happen, since we'd raise if not fitted. And if one of these attributes is non-existent after a fit, there clearly must be a bug somewhere.

I'm not sure why this is done. Is it a sticking point for this PR?

In any case, we do want to make it so that objects that were made in MNE 0.19 and earlier can still be read. If it's not clear what you need to do to write out and verify n_iter_ let me know and I can look

[hoechenberger]

Is it a sticking point for this PR?

No, not at all. I just happened to see this code and thought, oh, that looks kinda odd :)
Will try to implement the writing of n_iter_ in a similar way now, though, for the sake of consistency

[larsoner]

LGTM +1 for merge once the relevant tests pass.

I expect some unrelated failures due to NumPy wheels and also what should be fixed by #7335

[larsoner]

Thanks @hoechenberger