Skip to content

use get_data instead of ._data #128

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

Merged
merged 1 commit into from
Nov 20, 2022
Merged

use get_data instead of ._data #128

merged 1 commit into from
Nov 20, 2022

Conversation

sappelhoff
Copy link
Owner

@sappelhoff sappelhoff commented Nov 18, 2022

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

  • the PR has been reviewed and all comments are resolved
  • all CI checks pass
  • (if applicable): the PR description includes the phrase closes #<issue-number> to automatically close an issue
  • (if applicable): bug fixes, new features, or API changes are documented in whats_new.rst
  • (if applicable): new contributors have added themselves to the authors list in the CITATION.cff file

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

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
Copy link
Owner Author

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

@Sam54000
Copy link

Sam54000 commented Nov 19, 2022

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
Copy link
Owner Author

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!

@sappelhoff sappelhoff merged commit af85617 into master Nov 20, 2022
@sappelhoff sappelhoff deleted the data branch November 20, 2022 11:14
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.

Issue with RawBrainVision
3 participants