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

Add skip channels to EDF interface #1110

Merged
merged 2 commits into from
Oct 11, 2024
Merged

Add skip channels to EDF interface #1110

merged 2 commits into from
Oct 11, 2024

Conversation

h-mayorquin
Copy link
Collaborator

@h-mayorquin h-mayorquin commented Oct 10, 2024

Related to #1106

This adds the user to the possibility of skipping non-neural channels on the EDF interface.

Some relevant comments:

  • Can't add testing right now. The EDF by neo is blocked when reading the format. That means that it does not work well with the parallel machinery that we have in pytest. I have been meaning to fix this on the neo level or add a hack to our pytest machinery but other priorities come first (see EDF blocks access when io is opened. NeuralEnsemble/python-neo#1557).
  • Up for debate on how to expose the available channels so the user can pick for them. Ideally, this should be a static or class method but then because of the blocking problem described in the previous point this will not work in a script (getting the channel names requires opening the file). Maybe this is simpler to fix at the neo level by providing a method to closing the objects but I did not want that to get in the way.
  • I will open another PR to improve the friendliness of the error for multiple offsets. (Done here Add more friendly error when writing recording with multiple offsets #1111)

extractor_kwargs = source_data.copy()
extractor_kwargs.pop("channels_to_skip")
extractor_kwargs["all_annotations"] = True
extractor_kwargs["use_names_as_ids"] = True
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bendichter
This is necessary to display meaningful names that the user can suppress with the newly added parameter. Otherwise, the channel ids are "1", "2", "3", etc.

This is technically a breaking change because we are changing the way we add the data to the NWBFile but I also consider this an improvement:

image

@bendichter
Copy link
Contributor

@h-mayorquin this looks good! Can you confirm that this is able to read and convert the sample EDF file?

@h-mayorquin
Copy link
Collaborator Author

@h-mayorquin this looks good! Can you confirm that this is able to read and convert the sample EDF file?

Yes, can't attach the stub here but I will send it on slack.

@h-mayorquin h-mayorquin marked this pull request as ready for review October 10, 2024 20:35
@bendichter bendichter self-requested a review October 11, 2024 14:48
@h-mayorquin h-mayorquin merged commit 52cd6aa into main Oct 11, 2024
44 of 46 checks passed
@h-mayorquin h-mayorquin deleted the add_edf_skip_channels branch October 11, 2024 14:54
Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.57%. Comparing base (36464df) to head (99fd8d6).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
...onv/datainterfaces/ecephys/edf/edfdatainterface.py 90.90% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1110      +/-   ##
==========================================
+ Coverage   90.44%   90.57%   +0.13%     
==========================================
  Files         129      129              
  Lines        8055     8173     +118     
==========================================
+ Hits         7285     7403     +118     
  Misses        770      770              
Flag Coverage Δ
unittests 90.57% <90.90%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...onv/datainterfaces/ecephys/edf/edfdatainterface.py 97.56% <90.90%> (-2.44%) ⬇️

... and 2 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants