Skip to content
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 strings for marker descriptions #103

Open
cbrnr opened this issue Nov 18, 2022 · 7 comments · May be fixed by #118
Open

Support strings for marker descriptions #103

cbrnr opened this issue Nov 18, 2022 · 7 comments · May be fixed by #118
Labels
effort:medium estimated medium effort task enhancement New feature or request impact:medium estimated medium impact task
Milestone

Comments

@cbrnr
Copy link
Collaborator

cbrnr commented Nov 18, 2022

Currently, we only support integer marker descriptions for "Stimulus" and "Response" marker types. String descriptions are only possible with "Comment" types.

This is a serious restriction, because it can lead to loss of information. I suggest that we also allow string descriptions for "Stimulus" and "Response" markers, which would allow us to just use the original (e.g. mne.Annotations) description.

Note that the specs do not contain our restriction at all. I already discussed this with my contact at BrainProducts, he says that this is a convention originated from their recorder software (which uses individual trigger lines). The format itself does not mandate integer marker descriptions at all (which was already mentioned here).

@sappelhoff
Copy link
Member

he says that this is a convention originated from their recorder software (which uses individual trigger lines).

indeed, this is the main reason why we have this restriction currently. As long as this feature will be well tested and does not lead to compatibility issues, I fully agree with it.

@cbrnr
Copy link
Collaborator Author

cbrnr commented Nov 18, 2022

How do you want to handle this? I'd put no restrictions on type and description. Instead, I would write markers exactly as passed to the function (when passed a list of dicts, when passed an events array nothing needs to be adapted). So no conversions to "S 1" etc. when passing a list of dicts.

@sappelhoff
Copy link
Member

So no conversions to "S 1" etc. when passing a list of dicts.

but leave that conversion intact when an events array is passed? 🤔

I think that may be fine.

@cbrnr
Copy link
Collaborator Author

cbrnr commented Nov 18, 2022

Yes exactly. We might even consider splitting this into two parameters:

  • events: only accepts 2D arrays and has (some of) our conversions in place
  • markers: the list of dicts, which writes types and descriptions as is

@sappelhoff
Copy link
Member

okay for the above, but I wouldn't split events into two parameters as I find it more clear to have the controls of how to write this information in one place.

@sappelhoff sappelhoff added enhancement New feature or request impact:medium estimated medium impact task effort:medium estimated medium effort task labels Nov 21, 2022
@sappelhoff sappelhoff added this to the 0.8 milestone Nov 21, 2022
@cbrnr
Copy link
Collaborator Author

cbrnr commented Nov 25, 2022

If we disable all checks regarding marker type and description when passing a list of dicts, we run into the problem that many (all?) tests currently use events passed as a list of dicts.

Should we introduce a new parameter to enable writing arbitrary marker types/descriptions? Or should we adapt our tests and pass an array (which still does the checks)?

@sappelhoff
Copy link
Member

I would naively say let's try to avoid another parameter, but if you are about to do the work and see that that's completely unreasonable, I also trust you with that and will be happy to review either way!

@cbrnr cbrnr linked a pull request Feb 19, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort:medium estimated medium effort task enhancement New feature or request impact:medium estimated medium impact task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants