-
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
[MRG] auto-add event_id key for BAD_ACQ_SKIP #1258
[MRG] auto-add event_id key for BAD_ACQ_SKIP #1258
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1258 +/- ##
==========================================
- Coverage 97.61% 97.46% -0.16%
==========================================
Files 40 40
Lines 8685 8703 +18
==========================================
+ Hits 8478 8482 +4
- Misses 207 221 +14 ☔ View full report in Codecov by Sentry. |
IDK what codecov is complaining about: from the diff on their site it looks like 100% of the new lines are hit |
mne_bids/read.py
Outdated
if set(desc_without_id) & special_annots: | ||
for annot in special_annots: | ||
# use a value guaranteed to not be in use | ||
event_id = {annot: max(event_id.values()) + 1} | event_id |
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.
Could we perhaps go much higher? Like, + 10000 or so? To clearly separate those events from the rest
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.
@sappelhoff If you feel this is unnecessary, go ahead and merge
it was just a thought
also since we do something similar in MNE itself I believe, with super high event codes
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.
Did you mean this?
event_id = {annot: max(event_id.values()) + 1} | event_id | |
event_id = {annot: max(event_id.values()) + 10000} | event_id |
I think it's a reasonable point 👍
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.
done in aff3f57
I find the following suspicious:
see: https://app.codecov.io/gh/mne-tools/mne-bids/pull/1258 it's using 87eea28 as the BASE commit, which is really outdated 🤔 I would have expected it to use HEAD as the BASE commit (c87a823) |
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.
Thanks @drammock
all green except codecov (which as discussed is misbehaving here; PR coverage is 100%) |
Great, thanks, Dan! |
PR Description
closes #1257
write_raw_bids
will now allow the situation whereBAD_ACQ_SKIP
annotations are present in the raw file, but no corresponding key is present in the user-passedevent_id
dict.Merge checklist
Maintainer, please confirm the following before merging.
If applicable: