-
Notifications
You must be signed in to change notification settings - Fork 87
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
account for optional Z in time strs #1240
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1240 +/- ##
==========================================
- Coverage 97.61% 97.60% -0.02%
==========================================
Files 40 40
Lines 8685 8682 -3
==========================================
- Hits 8478 8474 -4
- Misses 207 208 +1 ☔ View full report in Codecov by Sentry. |
@sappelhoff I tried to give it a shot Code isn't super pretty but I think the logical flow is very clear Didn't run or write any tests yet |
I added some tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be good to go. Another example of a poorly designed part of BIDS, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a lot! Would you mind adding a changelog entry?
Very kind of you to include me in the Thanks, I think this is much saner behavior now!! |
yeah now worries, you started the PR after all :)
let's hope I / we didn't overlook something here … but I think in any case, we're behaving more correctly now than before! |
PR Description
fix a problem that was reported here: https://mne.discourse.group/t/how-to-import-fnirs-data-from-a-local-dataset/8481/3?u=sappelhoff
cc @hoechenberger
Merge checklist
Maintainer, please confirm the following before merging.
If applicable: