sappelhoff / pyprep

A Python implementation of the Preprocessing Pipeline (PREP) for EEG data
https://pyprep.readthedocs.io/en/latest/
MIT License
128 stars 30 forks source link

use get_data instead of ._data #128

Closed sappelhoff closed 1 year ago

sappelhoff commented 1 year ago

PR Description

fixes #127

Not really sure what the problem was over there, but the adjustments here are minor (get_data() should yield the same as ._data, except that it takes a tiny bit longer) and it'd fix the issue.

Merge Checklist

codecov-commenter commented 1 year ago

Codecov Report

Merging #128 (31afc3b) into master (c6b268b) will decrease coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
- Coverage   99.03%   99.02%   -0.02%     
==========================================
  Files           7        6       -1     
  Lines         725      717       -8     
==========================================
- Hits          718      710       -8     
  Misses          7        7              
Impacted Files Coverage Δ
pyprep/find_noisy_channels.py 100.00% <100.00%> (ø)
pyprep/utils.py 100.00% <100.00%> (ø)
pyprep/__init__.py

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

sappelhoff commented 1 year ago

@sam54000 when you install pyprep from this branch ("data", https://github.com/sappelhoff/pyprep/tree/data), does this fix your problem?

Sam54000 commented 1 year ago

I installed pyprep from the branch "data" it works. Also you right it is something to do with the preload: the mne-bids function to read raw doesn't have explicitely the preolad option so I assumed it was taking care of (I tried with the main branch). Then I figured that I have to specificaly pass the option through extra_params like this: mne_bids.read_raw_bids(bids_path=bids_path, verbose=False,extra_params={'preload' : True}). So yes if it is faster with ._data we should keep this as it is. Maybe changing the code to throw an error saying that raw needs to be preloaded if ._data doesn't exist? Thank you very much for your help and sorry for this

sappelhoff commented 1 year ago

Thanks for testing and the additional info! I think I will merge this fix, because the impact on performance is really negligible (the newly introduced get_data will not (!) be called hundreds of times in a loop). if you want to prepare a PR that introduces a check for preload, please be my guest!