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

variables renaming for consistency in prep.py + tests #197

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

patricefortin
Copy link
Member

@patricefortin patricefortin commented Sep 30, 2024

Fixed issues:

  • exceptions that were not properly raised in error flows

Improvements:

  • added freqs argument to prep.filt()
  • removed redundant ica fit that increased execution time
  • variables renaming for readability
  • use list comprehension
  • move existing tests on prep.py to test_prep.py
  • parameterize tests for testing multiple strategy/mode
  • add simple test for prep.filt()

@@ -347,7 +347,7 @@ def pair_connectivity(data: Union[list, np.ndarray], sampling_rate: int, frequen
elif type(frequencies) == dict:
values = compute_freq_bands(data, sampling_rate, frequencies)
else:
TypeError("Please use a list or a dictionary to specify frequencies.")
raise TypeError("Please use a list or a dictionary to specify frequencies.")
Copy link
Member Author

Choose a reason for hiding this comment

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

exceptions must be raised in order to interrupt the execution flow

for raw in raw_S:
raws.append(mne.io.Raw.filter(raw, l_freq=2., h_freq=None))

raws = [mne.io.Raw.filter(raw, l_freq=freqs[0], h_freq=freqs[1]) for raw in raw_S]
Copy link
Member Author

Choose a reason for hiding this comment

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

use list comprehension in multiple places to take advantage of type inference

" Index begins at zero. (If you do not want to apply"
" ICA on your data, do not enter nothing and press enter.)")
comp_number = input("Which IC do you want to use as a template?"
subject_id = input("Which participant ICA do you want"
Copy link
Member Author

Choose a reason for hiding this comment

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

variable renaming for consistency


return cleaned_epochs_ICA
if (len(subject_id) == 0 or len(component_id) == 0):
Copy link
Member Author

Choose a reason for hiding this comment

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

Invert code flow to simplify code (return early to reduce nested code)

threshold=0.9,
label='blink',
ch_type='eeg')
corrmap(icas,
Copy link
Member Author

Choose a reason for hiding this comment

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

Added plot as ICA_apply argument (to avoid plots launching GUI windows during tests). The presence/absence of plot argument here change the return type of corrmap, so we cannot always unpack fig_template and fig_detected. Anyway, they are not used.


from hypyp import prep

def test_filt():
Copy link
Member Author

Choose a reason for hiding this comment

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

new test

dict(method='fastica', fit_params=dict(tol=0.01)), # increase tolerance to converge
dict(method='infomax', fit_params=dict(extended=True))
])
def test_ICA(epochs, fit_kwargs):
Copy link
Member Author

Choose a reason for hiding this comment

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

test moved from test_stats.py

  • parameterize to test fastica
  • added amplitude comparison
  • added the plot=False to avoid launching GUI windows

dict(strategy='union'),
dict(strategy='intersection'),
])
def test_AR_local(epochs, AR_local_kwargs):
Copy link
Member Author

Choose a reason for hiding this comment

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

test moved from test_stats.py

  • parameterize for better coverage of intersection

dict(strategy='union'),
dict(strategy='intersection'),
])
def test_AR_local_exception(epochs, AR_local_kwargs):
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a working syntax for exception testing. The previous one was not testing the behavior properly

@@ -6,7 +6,6 @@
import numpy as np
import scipy
import mne
from hypyp import prep
Copy link
Member Author

Choose a reason for hiding this comment

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

moved prep related tests to test_prep.py

@patricefortin patricefortin marked this pull request as ready for review September 30, 2024 16:34
def filt(raw_S: list) -> list:
def filt(
raw_S: List[mne.io.Raw],
freqs: Tuple[Union[float, None], Union[float, None]] = (2., None)
Copy link
Member Author

Choose a reason for hiding this comment

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

added the "freqs" argument to complete the TODO in this function

@deep-introspection deep-introspection merged commit 396307b into ppsp-team:master Oct 1, 2024
3 checks passed
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.

2 participants