Support arbitrary markers #118
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.
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 |
| else: | ||
| # if cannot convert to int, we must use this as "Comment" | ||
| etype = "Comment" | ||
|
|
There was a problem hiding this comment.
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.
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. |
sappelhoff
force-pushed
the
main
branch
2 times, most recently
from
9379d40
to
baf68d7
Compare
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.typeis 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.