mne-tools / mne-tools/mne-python

[BUG] Several small ica bugs (+ first fixes)

Open
#5,967 5 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
3.5k
Forks
1.6k
Avg merge
1d 6h
Merged PRs (30d)
100

Description

I encountered a few minor bugs in ica:

Already fixed
  • ica._band_pass_filter (called (only) in ica.score_sources to filter sources and targets) uses the sfreq used when fitting the ica instead of the actual sfreq of the sources/targets (i.e. of raw).
  • ica._detect_artifacts is sorting scores in ascending rather than descending order for integer criterion arguments, contrary to what is promised in the docstring. This only affects the experimental detect_artifacts function.

The above 2 issues should be fixed in a first PR.

There are a few other things I would like to discuss first (should that be another issue, then?):

Checks for ica.exclude
Problem

The ica.exclude attribute is supposed to be a list of integers (while the exclude argument is supported to be "array-like"). However, neither is asserted and when providing an np.array instead, it is silently converted into a list.
BUT: Before that, strange things happen, if manually setting the ica.exclude attribute to a np.array (and exclude=None, otherwise both would have to have the same number of elements in order to not erroring): Random permutations of this array get added up numerically (instead of being appended), resulting in completely random components being removed, and for each call a different set of components (because of set().

Suggestions/Options
  1. The main culprit is a redundancy, which I think should be removed:
    a) This is called in the beginning of each apply...-method (by exclude = self._check_exclude(exclude)):
def _check_exclude(self, exclude):
    if exclude is None:
        return list(set(self.exclude))
    else:
        return list(set(self.exclude + exclude))
        # should instead be:
        # return list(set(list(self.exclude) + list(exclude))) # (see 2.b))

Now, self.exclude got permuted and copied to exclude.

b) Later, all the apply...methods call this:

def _pick_sources(self, data, include, exclude):
    """Aux function."""
# The following lines should be removed, since all the calling _apply_... functions do that already:
    if exclude is None:
        exclude = self.exclude
     else:
        exclude = list(set(self.exclude + list(exclude))) 
# ... or should at least be replaced with:
# exclude = self._check_exclude(exclude)

Here, the copied and permuted version in exclude (a list) gets added numerically to self.exclude (a np.array) ...

  1. Check
    a) Either assert self.exclude is a list and throw error or
    b) silently convert it to a list, as suggested above under 1 a)

--> What would be the preferred way?

Side note:

In contrast to exclude, picks has to be a np.array and bads has to be a list of strings. I think this has the potential to cause confusion not only for me. So, for me that would speak for option 2b) and to officially supporting both exclude and ica.exclude being both.

(Puhh, that was long ... ;))

Modify in place

(not a bug)
As I understood from some discussions, it is the general practice to modify large objects in place and providing a copy() method to the user.
I observed, that ica.get_sources always copies the (potentially big ...) data.
Should that be addressed, i.e. should the raw data in the inst object be replaced instead of returning a separate sources object?

OK, I guess I should stop here for this issue ;)

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the mentioned ICA entry points: _band_pass_filter and _detect_artifacts for the two reported fixes, then inspect _check_exclude, _pick_sources, the apply... methods, and get_sources. Determine the preferred exclude handling and scope with the unresolved discussion; done requires agreed behavior for these cases and verification of the reported bugs.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
machine-learning
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.