Support arbitrary markers

bids-standard / pybv

A lightweight I/O utility for the BrainVision data format, written in Python.

https://pybv.readthedocs.io/

BSD 3-Clause "New" or "Revised" License

21 stars 14 forks source link

Support arbitrary markers #118

Open cbrnr opened 8 months ago

cbrnr commented 8 months ago

Fixes #103. This is a big (breaking) change, I decided to tackle this as follows:

If the passed events is an np.ndarray, the previous conversion magic is still happening (i.e. type is set to "Stimulus" and integers are converted to "S 1" etc.).
If the passed events is a list of dicts, markers are written as is.

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.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (e1a6c79) 100.00% compared to head (bd0bb13) 100.00%. Report is 4 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #118 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 5 5 Lines 634 625 -9 ========================================= - Hits 634 625 -9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sappelhoff commented 8 months ago

Thanks Clemens, I will have time to review this later this week!

cbrnr commented 8 months ago

Here's something to consider: pybv can already export arbitrary markers as type Comment, so maybe there is no need for such a big change? Coming from MNE, the private _export_mne_raw() function already does the job (it sets the type of all markers to Comment).

sappelhoff commented 8 months ago

Coming from MNE, the private _export_mne_raw() function already does the job (it sets the type of all markers to Comment).

I assume you are referring to this?

https://github.com/bids-standard/pybv/blob/79dceaa02f6a2da77e8d5b27979ee3f378626137/pybv/_export.py#L126-L130

Here's something to consider: pybv can already export arbitrary markers as type Comment, so maybe there is no need for such a big change?

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.