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

[DRAFT] Redesign/datasets ICA addition #56

Open
wants to merge 51 commits into
base: develop-eeg
Choose a base branch
from

Conversation

vmcru
Copy link
Contributor

@vmcru vmcru commented Mar 5, 2025

added ica as dynamic item to preprocessing pipeline. uses BIDSPath and exposes method and other features. still has issues with the use of caching.

@vmcru vmcru marked this pull request as ready for review March 5, 2025 21:00
@bruAristimunha bruAristimunha self-assigned this Mar 6, 2025
@bruAristimunha
Copy link
Collaborator

Move this to the other PR.

Copy link
Collaborator

@Drew-Wagner Drew-Wagner left a comment

Choose a reason for hiding this comment

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

Thanks for this Victor! Here are my recommendations

Comment on lines 86 to 102
def process(
self, raw: mne.io.RawArray, raw_path: Union[str, Path]
) -> mne.io.RawArray:
"""Process raw data with ICA, computing or loading from cache."""

ica_path = self.get_ica_path(raw_path)

if not ica_path.exists():
ica = self.compute_ica(raw, ica_path)
else:
ica = mne.preprocessing.read_ica(ica_path, verbose="ERROR")

# Create a copy of the raw data before applying ICA
raw_ica = raw.copy()
ica.apply(raw_ica)

return raw_ica
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def process(
self, raw: mne.io.RawArray, raw_path: Union[str, Path]
) -> mne.io.RawArray:
"""Process raw data with ICA, computing or loading from cache."""
ica_path = self.get_ica_path(raw_path)
if not ica_path.exists():
ica = self.compute_ica(raw, ica_path)
else:
ica = mne.preprocessing.read_ica(ica_path, verbose="ERROR")
# Create a copy of the raw data before applying ICA
raw_ica = raw.copy()
ica.apply(raw_ica)
return raw_ica
@property
def dynamic_item(self):
@takes("raw", "fpath")
@provides("raw", "ica_path")
def process(
raw: mne.io.RawArray, fpath: Union[str, Path]
):
"""Process raw data with ICA, computing or loading from cache."""
ica_path = self.get_ica_path(fpath)
if not ica_path.exists():
ica = self.compute_ica(raw, ica_path)
else:
ica = mne.preprocessing.read_ica(ica_path, verbose="ERROR")
# Create a copy of the raw data before applying ICA
raw_ica = raw.copy()
ica.apply(raw_ica)
yield raw_ica
yield ica_path
return process

Copy link
Collaborator

Choose a reason for hiding this comment

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

function inside one function is not super nice

Comment on lines +100 to +107
bids_path.root = bids_path.root / ".." / "derivatives" / folder_name
# Keep the same base entities:
bids_path.update(
suffix="eeg", # override or confirm suffix
extension=".fif",
description=desc, # <-- This sets a desc=ica entity
check=True, # If you do not want BIDSPath to fail on derivative checks
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bids_path.root = bids_path.root / ".." / "derivatives" / folder_name
# Keep the same base entities:
bids_path.update(
suffix="eeg", # override or confirm suffix
extension=".fif",
description=desc, # <-- This sets a desc=ica entity
check=True, # If you do not want BIDSPath to fail on derivative checks
)
ica_path = bids_path.update(
processing="ica", suffix="ica"
)

check=True, # If you do not want BIDSPath to fail on derivative checks
)
# Make sure the folder is created
bids_path.fpath.parent.mkdir(parents=True, exist_ok=True)
Copy link
Collaborator

@bruAristimunha bruAristimunha Mar 25, 2025

Choose a reason for hiding this comment

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

Suggested change
bids_path.fpath.parent.mkdir(parents=True, exist_ok=True)
ica_path.mkdir(parents=True, exist_ok=True)

Comment on lines +111 to +112
ica_path = bids_path.fpath
metadata_path = ica_path.with_suffix(".json")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ica_path = bids_path.fpath
metadata_path = ica_path.with_suffix(".json")
metadata_path = bids_path.update(
suffix="metaica", extension=".json"
)

Comment on lines +143 to +149
if self.filter_params is not None:
# Apply high-pass filter only if filter parameters are provided
raw_filtered = raw.copy()
raw_filtered.filter(**self.filter_params)
else:
# Use raw data directly if no filtering is needed
raw_filtered = raw
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if self.filter_params is not None:
# Apply high-pass filter only if filter parameters are provided
raw_filtered = raw.copy()
raw_filtered.filter(**self.filter_params)
else:
# Use raw data directly if no filtering is needed
raw_filtered = raw
raw.info['h_freq']

n_components=self.n_components,
method=self.method,
random_state=self.random_state,
**self.fit_params,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
**self.fit_params,
**self._fit_params,

@@ -95,10 +95,12 @@ def __init__(
data,
preload=False,
verbose=None,
# ica_processor: Optional[ICAProcessor] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# ica_processor: Optional[ICAProcessor] = None,

@vmcru vmcru changed the base branch from redesign/datasets to develop-eeg March 25, 2025 17:10
vmcru and others added 2 commits March 25, 2025 17:53
description update

Co-authored-by: Bru <[email protected]>
description update 2

Co-authored-by: Bru <[email protected]>
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.

3 participants