Skip to content
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

MRG, FIX: Check reject_by_annotation in ica.plot_properties #8103

Merged
merged 7 commits into from
Aug 10, 2020

Conversation

yh-luo
Copy link
Contributor

@yh-luo yh-luo commented Aug 7, 2020

Fix #8101 .

Because reject_by_annotation in ica.fit() defaults to True, reconstruction of source in plot_properties should take care of that or the error occurs due to the incorrect number of epochs.

TODO

  • Fix ica.plot_properties
  • Fix make_fixed_length_epochs
  • Update documentation of reject_by_annotation using docdict
  • Update what's new

Before this PR

After adding annotations

---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
<ipython-input-4-d42215032b75> in <module>
      6 
      7 ica.fit(raw)
----> 8 ica.plot_properties(raw, picks=[0, 1])

~/env/mne/lib64/python3.7/site-packages/mne/preprocessing/ica.py in plot_properties(self, inst, picks, axes, dB, plot_std, topomap_args, image_args, psd_args, figsize, show, reject)
   1631                                    topomap_args=topomap_args,
   1632                                    image_args=image_args, psd_args=psd_args,
-> 1633                                    figsize=figsize, show=show, reject=reject)
   1634 
   1635     @copy_function_doc_to_method_doc(plot_ica_sources)

~/env/mne/lib64/python3.7/site-packages/mne/viz/ica.py in plot_ica_properties(ica, inst, picks, axes, dB, plot_std, topomap_args, image_args, psd_args, figsize, show, reject)
    403 
    404     epochs_src = ica.get_sources(inst_rejected)
--> 405     data = epochs_src.get_data()
    406 
    407     ica_data = np.swapaxes(data[:, picks, :], 0, 1)

~/env/mne/lib64/python3.7/site-packages/mne/epochs.py in get_data(self, picks, item)
   1412             A view on epochs data.
   1413         """
-> 1414         return self._get_data(picks=picks, item=item)
   1415 
   1416     @property

<decorator-gen-182> in _get_data(self, out, picks, item, verbose)

~/env/mne/lib64/python3.7/site-packages/mne/epochs.py in _get_data(self, out, picks, item, verbose)
   1343                     else:
   1344                         epoch_noproj = None
-> 1345                         epoch = self._data[idx]
   1346                 else:  # from disk
   1347                     epoch_noproj = self._get_epoch_from_raw(idx)

IndexError: index 28 is out of bounds for axis 0 with size 28

Figure_1


After this PR

Before adding annotations

import os

import mne

sample_data_folder = mne.datasets.sample.data_path()
sample_data_raw_file = os.path.join(sample_data_folder, 'MEG', 'sample',
                                    'sample_audvis_raw.fif')
raw = mne.io.read_raw_fif(sample_data_raw_file)
raw.crop(tmax=60)

raw.load_data()
raw.filter(1, 40, n_jobs='cuda')

ica = mne.preprocessing.ICA()

ica.fit(raw)
ica.plot_properties(raw, picks=[0, 1])

image
image

After adding annotations

# refitting with annotations
a = mne.Annotations(onset=[3],
                    duration=[1],
                    description=['BAD'])
raw.set_annotations(a)

ica.fit(raw)
ica.plot_properties(raw, picks=[0, 1])

image
image

This PR fixes the bug and the results are not influenced.

@yh-luo yh-luo changed the title FIX: Check reject_by_annotation in ica.plot_properties WIP, FIX: Check reject_by_annotation in ica.plot_properties Aug 7, 2020
@agramfort
Copy link
Member

can you update or add a test @yh-luo ?

@yh-luo
Copy link
Contributor Author

yh-luo commented Aug 8, 2020

@agramfort I will work on it!

@yh-luo yh-luo force-pushed the properties branch 2 times, most recently from 991192a to 7779b95 Compare August 8, 2020 11:58
@yh-luo
Copy link
Contributor Author

yh-luo commented Aug 8, 2020

The bug is actually caused by mne.epochs.make_fixed_length_epochs. This PR added test for it too.

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also can you see why CIs are failing?

code looks good !

mne/epochs.py Outdated
reject_by_annotation : bool
Whether to reject based on annotations. If ``True`` (default), epochs
overlapping with segments whose description begins with ``'bad'`` are
rejected. If ``False``, no rejection based on annotations is performed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you factorize this docstring using docdict to avoid a copy paste?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of functions duplicate the documentation of reject_by_annotation. I updated most of them too (except those default to False).

@larsoner
Copy link
Member

larsoner commented Aug 8, 2020

CircleCI can be ignored, it's fixed by #8109 (will restart once that's merged)

@yh-luo yh-luo changed the title WIP, FIX: Check reject_by_annotation in ica.plot_properties MRG, FIX: Check reject_by_annotation in ica.plot_properties Aug 10, 2020
@yh-luo yh-luo requested a review from agramfort August 10, 2020 09:15
@agramfort
Copy link
Member

@yh-luo please add a what's new entry and then +1 for MRG. thx

@larsoner larsoner merged commit 9c7788a into mne-tools:master Aug 10, 2020
@larsoner
Copy link
Member

Thanks @yh-luo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ica.plot_properties error bug
4 participants