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

New feature for removing heart artifacts from EEG or ESG data using a Principal Component Analysis - Optimal Basis Sets (PCA-OBS) algorithm #13037

Open
wants to merge 52 commits into
base: main
Choose a base branch
from

Conversation

steinnhauser
Copy link

What does this implement/fix?

Our PR implements the PCA-OBS algorithm for removal of heart-artefacts from EEG or ESG datasets. The PCA-OBS algorithm was originally designed to remove the ballistocardiographic artefact in simultaneous EEG-fMRI. Here, it has been adapted to remove the delay between the detected R-peak and the ballistocardiographic artefact such that the algorithm can be applied to remove the cardiac artefact in EEG (electroencephalogrpahy) and ESG (electrospinography) data.

Positive and negative tests have been implemented for the feature, and we have also included an example script which is implemented using data from openneuro (dataset ID: ds004388).

Authors:
Emma Bailey [email protected]
Steinn Hauser Magnusson [email protected]

emma-bailey and others added 30 commits September 1, 2024 12:57
Updating master with most recent package features
feat: add initial source code
…ove more unused variables and imports, add some types
…et data from, how we call functions, how we assert outputs
Update PCA OBS fork with most recent changes in main MNE repo
…ogging to use mne logger instead of prints, add wrapper method in front of private _pca_obs method to handle parallel processing
steinnhauser and others added 16 commits November 25, 2024 09:13
refactor: Initial cleanup of new PCA OBS source code
… function input, ad tests for copying original data and comparing to data modified in-place, add window size checks and remove generic try-except blocks

BREAKING CHANGE
…ndices, add sanity checks for input values, add negative-test which verifies proper exceptions when bad data is passed to function
test: add initial test structure, missing validation of post-hear-art…
@drammock
Copy link
Member

Thanks for the contribution! I haven't looked at the diff in any detail yet. Here's a summary of the failing CIs:

new functions not added to appropriate file in doc/api/

mne\tests\test_docstring_parameters.py:354: in test_documented
    raise AssertionError(
E   AssertionError: 1 new public member missing from doc/python_reference.rst:
E   mne.preprocessing : mne.preprocessing.pca_obs.apply_pca_obs

test is missing a requires_pandas decorator and/or pytest.importorskip(pandas)

___________ ERROR collecting mne/preprocessing/tests/test_pca_obs.py ___________
ImportError while importing test module '/home/vsts/work/1/s/mne/preprocessing/tests/test_pca_obs.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/hostedtoolcache/Python/3.12.7/x64/lib/python3.12/site-packages/_pytest/python.py:493: in importtestmodule
    mod = import_path(
/opt/hostedtoolcache/Python/3.12.7/x64/lib/python3.12/site-packages/_pytest/pathlib.py:587: in import_path
    importlib.import_module(module_name)
/opt/hostedtoolcache/Python/3.12.7/x64/lib/python3.12/importlib/__init__.py:90: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1387: in _gcd_import
    ???
<frozen importlib._bootstrap>:1360: in _find_and_load
    ???
<frozen importlib._bootstrap>:1331: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:935: in _load_unlocked
    ???
/opt/hostedtoolcache/Python/3.12.7/x64/lib/python3.12/site-packages/_pytest/assertion/rewrite.py:184: in exec_module
    exec(co, module.__dict__)
mne/preprocessing/tests/test_pca_obs.py:8: in <module>
    import pandas as pd
E   ModuleNotFoundError: No module named 'pandas'

don't download data within a tutorial/example file (our CIs pre-fetch / cache most of the data we need for testing / doc building); we can discuss what the best way forward is here (use another dataset that we already cache; add ds004388 to our cache; something else?)

Traceback (most recent call last):
  File "/home/circleci/python_env/lib/python3.12/site-packages/sphinx/cmd/build.py", line 514, in build_main
    app.build(args.force_all, args.filenames)
  File "/home/circleci/python_env/lib/python3.12/site-packages/sphinx/application.py", line 383, in build
    self.events.emit('build-finished', None)
  File "/home/circleci/python_env/lib/python3.12/site-packages/sphinx/events.py", line 404, in emit
    results.append(listener.handler(self.app, *args))
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/circleci/python_env/lib/python3.12/site-packages/sphinx_gallery/gen_gallery.py", line 1538, in summarize_failing_examples
    raise ExtensionError(fail_message)
sphinx.errors.ExtensionError: Here is a summary of the problems encountered when running the examples:

Unexpected failing examples (1):

    ../examples/preprocessing/esg_rm_heart_artefact_pcaobs.py failed leaving traceback:

    Traceback (most recent call last):
      File "/home/circleci/project/examples/preprocessing/esg_rm_heart_artefact_pcaobs.py", line 52, in <module>
        on.download(
      File "/home/circleci/python_env/lib/python3.12/site-packages/openneuro/_download.py", line 903, in download
        asyncio.run(coroutine)
      File "/usr/lib/python3.12/asyncio/runners.py", line 194, in run
        return runner.run(main)
               ^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/asyncio/runners.py", line 118, in run
        return self._loop.run_until_complete(task)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/asyncio/base_events.py", line 687, in run_until_complete
        return future.result()
               ^^^^^^^^^^^^^^^
      File "/home/circleci/python_env/lib/python3.12/site-packages/openneuro/_download.py", line 558, in _download_files
        outfile.parent.mkdir(parents=True, exist_ok=True)
      File "/usr/lib/python3.12/pathlib.py", line 1317, in mkdir
        self.parent.mkdir(parents=True, exist_ok=True)
      File "/usr/lib/python3.12/pathlib.py", line 1317, in mkdir
        self.parent.mkdir(parents=True, exist_ok=True)
      File "/usr/lib/python3.12/pathlib.py", line 1313, in mkdir
        os.mkdir(self, mode)
    PermissionError: [Errno 13] Permission denied: '/data'

@agramfort
Copy link
Member

@steinnhauser can you share a plot where we can see that this does better than let's say SSP projections that MNE has for such needs?

@steinnhauser
Copy link
Author

@drammock thank you for your feedback! I've added some more commits to address them.

new functions not added to appropriate file in doc/api/

I've tried to add the proper import in 8f4dcdd. If there are any patterns here the don't match your conventions feel free to let me know 🙂

test is missing a requires_pandas decorator and/or pytest.importorskip(pandas)

I've added pytest.importorskip("pandas") to both tests in 826afec. ✔️

don't download data within a tutorial/example file (our CIs pre-fetch / cache most of the data we need for testing / doc building); we can discuss what the best way forward is here (use another dataset that we already cache; add ds004388 to our cache; something else?)

This sounds like an interesting discussion, since the ds004388 dataset is quite large. Not sure how easy it would be for you to add this to your cache. We could maybe add one or two of the participants - the dataset contains 40 but I don't think they are all relevant for the example.

@emma-bailey
Copy link

@agramfort It's actually not better - we have a current preprint on the topic here which compares PCA-OBS, SSP and ICA approaches at the spinal level:
https://www.biorxiv.org/content/10.1101/2024.09.05.611423v1

The advantage lies in the fact you can run PCA-OBS even if you only record with a single electrode, or a very minimal array, which is quite common in spinal recordings in particular. When we looked at using SSP at the spinal level, you need ~6 projections to adequately remove the artefact, so you need to be recording with a decent number of electrodes in that case.

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.

5 participants