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

[MRG] Improve check of scans.tsv for BrainVision files #1034

Merged
merged 11 commits into from
Aug 11, 2022

Conversation

teonbrooks
Copy link
Member

PR Description

This PR closes #1033.

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"

mne_bids/read.py Outdated Show resolved Hide resolved
mne_bids/read.py Outdated Show resolved Hide resolved
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

would it be possible to add a test or is it too much to ask?

@teonbrooks teonbrooks changed the title [MRG] Update read.py [WIP] Update read.py Aug 2, 2022
@teonbrooks
Copy link
Member Author

would it be possible to add a test or is it too much to ask?

@agramfort, yep, i can do that. i renamed it to wip, and i'll write a test in the morning. noticed this at neurohackademy when working through an ieeg example

@sappelhoff sappelhoff changed the title [WIP] Update read.py [WIP] Improve check of scans.tsv for BrainVision files Aug 2, 2022
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

looks like you have some Windows issue:

ValueError: 'eeg\\sub-01_ses-01_task-testing_acq-01_run-01_eeg.vhdr' is not in list

probably as_posix needs to be called on the Path somewhere, to normalize to / instead of \ on win.

mne_bids/tests/test_read.py Outdated Show resolved Hide resolved
doc/whats_new.rst Outdated Show resolved Hide resolved
Co-authored-by: Stefan Appelhoff <[email protected]>
@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #1034 (085c382) into main (16a7b0c) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 085c382 differs from pull request most recent head cdaec9a. Consider uploading reports for the commit cdaec9a to get more accurate results

@@            Coverage Diff             @@
##             main    #1034      +/-   ##
==========================================
- Coverage   95.19%   95.17%   -0.02%     
==========================================
  Files          25       25              
  Lines        3805     3811       +6     
==========================================
+ Hits         3622     3627       +5     
- Misses        183      184       +1     
Impacted Files Coverage Δ
mne_bids/read.py 96.05% <100.00%> (+0.05%) ⬆️
mne_bids/write.py 97.04% <0.00%> (-0.11%) ⬇️

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

@teonbrooks
Copy link
Member Author

looks like you have some Windows issue:

ValueError: 'eeg\\sub-01_ses-01_task-testing_acq-01_run-01_eeg.vhdr' is not in list

probably as_posix needs to be called on the Path somewhere, to normalize to / instead of \ on win.

so i'm looking into this and it seems like we read in the tsv file but we don't change filename from str to a filepath obj that would be insensitive to the os. should we be working with filepath all over the place?

also, if you see an easy fix, please let me know, i keep bumping into different windows errors on the testing front. not sure if i have any more time to debug it at the moment

@sappelhoff sappelhoff added this to the 0.11 milestone Aug 10, 2022
@agramfort
Copy link
Member

ok all good !

CI failure with doc is also in main branch

@agramfort agramfort merged commit 46da987 into mne-tools:main Aug 11, 2022
@agramfort
Copy link
Member

thanks @teonbrooks !

@teonbrooks teonbrooks deleted the scans branch August 11, 2022 14:08
@teonbrooks teonbrooks changed the title [WIP] Improve check of scans.tsv for BrainVision files [MRG] Improve check of scans.tsv for BrainVision files Aug 11, 2022
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.

[BUG] parsing scans.tsv is currently too restrictive for BrainVision files
3 participants