Skip first "New Segment" BrainVision marker

[cbrnr]

Fixes #13095. This PR skips the first "New Segment" BrainVision marker, as it only contains the recording time (which is already available in info["meas_date"]). I decided to not do anything with subsequent "New Segment" markers, as such changes could be unexpected.

[sappelhoff]

Just a note that I did agree to this change under the assumption that "New Segment" markers are now interpreted as BAD_ACQ_SKIP events (see #13095 (comment)). Which would have been a principled way of interpreting "New Segment" markers.

However, with the current changes, we are just removing the first marker.

It makes me a bit uncomfortable that we are basically taking events that are in the file and deciding which ones are irrelevant and which ones aren't. This should be a user choice, and we have very straight forward ways of filtering which events to keep and which not (proper use of event_id).

There are also several other events that are commonly not used, like Mk2=Comment,ControlBox is not connected via USB,41,1,0, see for example https://gin.g-node.org/sappelhoff/mpib_ecomp_sourcedata/src/master/sub-01/dual/sub-01_stream-dual.vmrk#L13 -- why would we then not remove those, too?

[cbrnr]

I think the answer is simple: we are not discarding the information contained in this first New Segment marker (the recording time), whereas there would be no other place to store the information from that other example you're mentioning.

[cbrnr]

But yes, I'm also fine with converting all New Segment markers to BAD_ACQ_SKIP, but where would we store the datetime in those later markers then? We'd be losing information, which we should avoid.

[sappelhoff]

yes, the logic is solid. Maybe this comes down to my personal preference of having the annotations in mne correspond to what I can see when opening my VMRK file in a code editor. But I realize that probably a minority of users feel this way, so ok from me to go ahead with it.

[drammock]

revert. This makes it sound like "list of str or tuple of str" but "list of int" is also supported

[cbrnr]

I copied it from the function, so this should be changed there as well then.

[cbrnr]

I replaced it with something more explicit, I hope this is fine now. It's also in the description of the misc parameter now (which I didn't touch before).

[drammock]

Do we need a maxsplit=1 here? Can the text after the market tour marker type ever contain a slash?

[cbrnr]

Technically both BrainVision marker types and descriptions can contain /, which would break our logic here. However, maxsplit=1 doesn't save us, because there could be one or more / in the description. Currently, we work under the assumption that neither the type nor the description of a marker contains a /. If we want to be on the safe side, we should replace all / before converting to annotations, after which maxsplit=1 is not necessary anymore. Do you want me to do this here?

[drammock]

wouldn't this be a possible regression then? The old way which did:

if not ignore_marker_types:
    description.append(mtype + "/" + mdesc)
else:
    description.append(mdesc)

didn't have a problem handling markers or descriptions that contained /

[cbrnr]

You are absolutely right! I implemented a more robust version, LMK if you're happy with it.