-
Notifications
You must be signed in to change notification settings - Fork 14
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
Support arbitrary markers #118
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 634 625 -9
=========================================
- Hits 634 625 -9 ☔ View full report in Codecov by Sentry. |
Thanks Clemens, I will have time to review this later this week! |
Here's something to consider: pybv can already export arbitrary markers as type |
"Comment"}`` (defaults to ``"Stimulus"``). Additional types like the | ||
known BrainVision types ``"New Segment"``, ``"SyncStatus"``, etc. are | ||
currently not supported. | ||
The type of the event (defaults to ``"Stimulus"``). |
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.
Stimulus, Response, and Comment are "standard" BrainVision types. I would not cut them out of the description here but rather relax the language from MUST to MAY or similar.
The description of the event. Must be a non-negative int when `type` | ||
(see below) is either ``"Stimulus"`` or ``"Response"``, and may be a str | ||
when `type` is ``"Comment"``. | ||
- ``"description"`` : str |
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.
may also be int, IMHO
else: | ||
# if cannot convert to int, we must use this as "Comment" | ||
etype = "Comment" | ||
|
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 removing this would result in some lines that are not pretty, like:
Mk2=Comment,Stimulus/S253,487,0,0
when it could also (more sensibly, as currently done) be:
Mk2=Stimulus,S253,487,0,0
When passing a list of dict to `events`, the event ``type`` that can be passed is | ||
currently limited to one of ``{"Stimulus", "Response", "Comment"}``. The BrainVision | ||
specification itself does not limit event types, and future extensions of ``pybv`` | ||
may permit additional or even arbitrary event types. |
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.
Fine to remove this, but I would somewhere keep a documentation that "Stimulus", "Response", and "Comment" are the classic types in a BrainVision file that is written by BrainVision Recorder.
I assume you are referring to this? Lines 126 to 130 in 79dceaa
I personally am able to write all data that I want to write. I thought your argument in #103 convincing so I am fine if you really need this change. But if the changes here are not a big advantage for you, then we should just leave it as is IMHO. |
Fixes #103. This is a big (breaking) change, I decided to tackle this as follows:
np.ndarray
, the previous conversion magic is still happening (i.e.type
is set to"Stimulus"
and integers are converted to"S 1"
etc.).This means that if someone needs to write "old-style" markers with custom types (such as
"Response"
,"Comment"
, etc.), they must manually create a corresponding list of dicts.I'm not sure if we need to change anything in MNE-Python, but I don't think so. It reads descriptions and types and combines them to "Type/Description", which I think should continue to work. The events from annotations heuristics will only work for "old-style" annotations, but that's OK I think.