Feature: Bit behaviour for process_evt_read.m

[rcassani]

This PR addresses the issue pointed by @Moo-Marc in the Forum post: https://neuroimage.usc.edu/forums/t/reading-ctf-digital-stim-events-in-bit-mode/34897

• Removes the trigger-shift corrections. This was introduced in 9329fa0 along side improvements in reading CTF triggers.
• Keep all changes in tracks (x→y, 0→x and x→0), not only from 0→x
• For the "Bit" option and Not "Accept zeros as trigger values". The nBit event group registers the set (0→1) of nBit.
• For the "Bit" option and "Accept zeros as trigger values". The nBit event group registers the set (0→1) and reset (1→0) of the nBit. However these two events will have the same event label. As a former improvement we could create two event groups: nBit-Set and nBit-Reset

[ftadel]

This doesn't work. Removing these three lines alters dramatically the behavior of the reader and makes the cortico-muscular coherence impossible to follow, as a lot more events are now detected.
https://neuroimage.usc.edu/brainstorm/Tutorials/CorticomuscularCoherence#Event_markers

Before/after:

[rcassani]

This behaviour is linked to cases where the edge has two (or even three!) steps. Verified on the CMC data and a file @Moo-Marc shared. Two examples, x-axis are samples, y-axis the value of the Stim (or UDIO001) channels.

• Two step falling edge
  

• Overshot Two step falling edge
  

This transition "triggers " are avoided by setting the option "Reject events shorter than X samples" to 2. I've tested this config with the CMC tutorial data (a54ac2a)

[Moo-Marc]

I tested with our bit-wise data. It works well with "reject" set to 0 samples, but otherwise it rejected a lot of events it shouldn't (5% to 50% depending on the bit). Maybe it's because of the way the data is processed in blocks? Or it doesn't apply that filter on each bit channel independently?

[ftadel]

We discussed this issue during yesterday's meeting.
I think there are problems with the proposed changes. This PR would change completely the way the events are detected, causing major backward compatibility issues. The patch proposed later by Raymundo is only a hack to make it match the previous results in one specific cases.

Fixed trigshift:

• @Moo-Marc argued that the computation of trigshift as trigshift = fix(sFile.prop.sfreq * 9/1200); is incorrect, it should instead be a fixed value of 4.
• As a fix, @Moo-Marc decided to completely remove this trigshift parameter and the associated line tracks = upflank(1:(end-trigshift)).*tracks((1+trigshift):end);. This solution does not match the initial problem (fixed trigshift) and causes lots of detection issues downstream (the multiple detections @rcassani mentioned, that require extra hacks).
• As these lines were written 20 years ago by Robert Oostenveld in FieldTrip function read_ctf_trigger, and have been used an infinite amount of times by all the CTF/FieldTrip users since then, I suggest we give it a second observation before simply deleting them.
• @Moo-Marc Please open an issue on the FieldTrip github repository to discuss this point, citing all the references you need to ask why this trigshift is not set to a fixed value of 4 in read_ctf_trigger

Detection of events only from zero:

• This concerns only the second part of the test upflank = [0 (diff(tracks)>0 & tracks(1:(end-1))==0)]; => By removing & tracks(1:(end-1))==0) we can decide whether we want to impose the actual value of tracks to be zero before the detection of the change.
• This is indeed not how we would expect this detector to work by default when detecting bit changes, as the value is not likely going down to zero. One solution would be to remove this second part only when the bit option is selected.
• Another solution would be to make it a new option of the process (e.g. "Detect only value changes from zero (CTF only)"), which would be enabled by default in order to preserve the current behavior of the reader. However, it is not clear to me in which context which is not a bitwise detection this option would be interesting.
• @Moo-Marc Please describe what kind of trigger box attached to a CTF system would generate integer triggers jumps between two non-zero values, that can't be interpreted as bit changes. For this topic too, I could be interested in hearing from the FieldTrip developers.

[Moo-Marc]

Hi François,

Trig shift: This was basically an undocumented(?) small time shift applied to digital CTF triggers recreated in Brainstorm, right? Is the same shift applied when loading events from the MarkerFile? If not, then it's another reason to remove it. I can discuss it with Fieldtrip, but it's possible the specific delay changed with the different CTF electronics versions. It was your suggestion that "we completely get rid of this shift", and deal with it in the delay tutorial. In fact, it's something that probably applies to other systems, any that downsamples somewhere in the hardware or software processing.

Event detection: bit-wise or not doesn't matter here, it depends on the values chosen by the task designer. In particular, assuming an 8-bit channel, if they have more than 8 distinct event types, they'd have to use decimal values from 0-255 instead of 8 independent bits. Simple example: a task with various events. The value on the digital channel is changed at each event: trial start = 0, first step = 1, second event = 10 or 20 depending on condition, response prompt = 30, response = 100 or 101. Or if it's designed bit-wise: start = 0, first step = 1, second = 2 or 4, prompt = 8, response = 16 or 32. So the stim channel looks like a staircase for each trial, but in the first case the numbers shouldn't be treated bit-wise. Now, when we use bit-wise and after the channel is converted to a set of bit "tracks", of course each event goes from 0 to 1, so the "from 0" criteria is logical, but it's also redundant at this stage.

Transitions in general happen in the middle of a single sample, and for CTF, the channel will "and" the bits (initially collected at something like 192kHz) and we end up with larger values at these transition samples. This is normal and the existing functionality "reject events shorter than 2 samples" is a very good solution to dealt with this. If we wanted to be even more careful, I guess we could compare these one-sample values to the bit-and of the previous and following values, and "clean" the transitions.

As to the impact of fixing this: I'd guess many people don't typically use these functions to generate events because they're created at collection time with a properly customized CTF configuration. Which is probably why nobody complained that bit-wise didn't work. But if the functionality is there, I think it's worth being able to recreate all the markers in Brainstorm, not just a subset of cases.

[Moo-Marc]

Fieldtrip issue opened: fieldtrip/fieldtrip#2030

[Moo-Marc]

From Robert's reply, my previous understanding was wrong. I had another look at the code and it's not so exactly doing a time shift, but avoiding transition values: it finds when the channel value departs from 0, and then takes the channel value a few samples later as the "correct" event value. It was not to avoid the bit-and that we were talking about, but rather if the different bit changing are not all synchronized. The effect is similar to our "reject events shorter than X samples" with X equivalent to 7.5 ms, but it also "shifts" the stable event that occurs after a short "transition event" to the time of the transition start.

I would suggest replacing that fixed duration "forward looking" feature, with a slightly modified "reject events shorter than": Filter out unwanted transition values, replacing them with the subsequent "stable value", hence similarly shifting the time of the stable events earlier. It's almost the same behavior as before for 0 to X transitions (but it can be longer than before if there are many different short duration values in a row - probably never happens), but it would also work for X to Y transitions.

[ftadel]

trial start = 0, first step = 1, second event = 10 or 20 depending on condition, response prompt = 30, response = 100 or 101
[...]
So the stim channel looks like a staircase for each trial

Is it a typical thing that the stim channel doesn't go back to zero after each event is generated?
I feel like in most recordings I processed, each event generation leads to a few samples with the event code, and then goes back to zero.
(this question has no impact on the decisions here, this is only for my information - if we need to handle the two cases, then we need to add an option).

This is normal and the existing functionality "reject events shorter than 2 samples" is a very good solution to dealt with this.

In the files of the introduction tutorials (downsampled at 600Hz), the events are all 1-sample long, therefore we should keep zero as the default for this parameter.

I would suggest replacing that fixed duration "forward looking" feature, with a slightly modified "reject events shorter than": Filter out unwanted transition values, replacing them with the subsequent "stable value", hence similarly shifting the time of the stable events earlier. It's almost the same behavior as before for 0 to X transitions (but it can be longer than before if there are many different short duration values in a row - probably never happens), but it would also work for X to Y transitions.

I'm sorry, I'm a bit lost in these technical details...
Could you please push all the modifications you think are necessary to this PR?
(keeping in mind that: 1) if some changes alter the results, it would be better to introduce new options and 2) it's better to submit only changes that are really needed for a specific use case)

[Moo-Marc]

Is it a typical thing...

It's not uncommon. Though I agree using short pulses (and independent bits when possible) has advantages and may be more common in experienced people.

In the files of the introduction tutorials (downsampled at 600Hz), the events are all 1-sample long, therefore we should keep zero as the default for this parameter.

Actually, to maintain the current behavior, the default should be 7.5 ms (but I'd still vote for 2 or 0). In this example, the events are not detected with the current code. By looking forward 7.5 ms the value is 0 again. And even with "accept 0 values", they are rejected. Robert did mention that they always used pulses at least 10 ms, hence it was not an issue for them.

By the way, I also noticed that a bunch of pulses are missing in the stim digital channel in this downsampled data. I would suggest that the downsampling procedure be modified for digital channels the same way CTF does it: bit-and of all values in the new sample period. This ensures no short events are lost.

Could you please push

I'll discuss with @rcassani first since he's now more familiar with the code. One of us will do that.

Final comment: my suggestion above is meant to keep the same behavior, while replacing the fixed 7.5 ms by the "shorter than" parameter. Otherwise, I would have suggested to just remove this "shift correction" entirely. We know that digital triggers are not the best for timing anyway as we discuss in the delay tutorial. For any task where the timing must be precise within one or two samples, one should use a different method to get the timing anyway (e.g. photodiode, sound recording, etc).

[ftadel]

Actually, to maintain the current behavior, the default should be 7.5 ms (but I'd still vote for 2 or 0). In this example, the events are not detected with the current code. By looking forward 7.5 ms the value is 0 again.

Ouch... indeed, it works in the tutorial dataset only because the files are notch-filtered first, and their format is "BST-BIN" instead of "CTF-CONTINUOUS". Detecting before filtering does not create any event.

I guess it would be important to obtain coherent behaviors for the original or filtered files, right?
And therefore removing the behaviors specific to the CTF file format, as you initially suggested?

But then, we need to maintain the behavior for the older files from the cortico-muscular coherence tutorial (which is the reason why I initially added this code, after checking why it was correctly read in the FieldTrip tutorial).
This trigshift thing could be an extra option, disabled by default and enabled mostly only for the coherence tutorial?
Or another equivalent solution like the ones you described in your previous posts?

By the way, I also noticed that a bunch of pulses are missing in the stim digital channel in this downsampled data. I would suggest that the downsampling procedure be modified for digital channels the same way CTF does it: bit-and of all values in the new sample period. This ensures no short events are lost.

Indeed, a large amount of triggers are lost (e.g. 170 standard triggers instead of 200...)
But I think these files were downsampled using the CTF software, by Beth at the MNI.
Do you have other ways to downsample the original files for the tutorials? (they are available in the file sample_auditory.zip in the download page of the Brainstorm website)

Thanks!

[rcassani]

@Moo-Marc, at this point, process_evt_read works as discussed last Friday. Can you test it on your side?

[rcassani]

In the data for the coherence tutorial, there are cases where the transition between events takes 2 samples. Since the triggers of interest in that data last 15 ms, I updated the "Reject events shorter than X samples" parameter to 10 ms (12 samples at 1200 Hz).

[Moo-Marc]

Looks good to me. Tested the bit-wise mode.

[Moo-Marc]

We should add a short description for the "reject events shorter than X samples" option to the Stim Delay Tutorial.
Examples of when it may be useful:

1. Individual bits not changing exactly at the same time;
2. Values added (bit-wise) at transitions between 2 non-zero values when downsampling (e.g. CTF);
3. "Button bouncing" (non-ideal behavior of any switch which generates multiple transitions of a single input)

anything else?

[ftadel]

I tested the new version vs. the old one on all the tutorial datasets that use this function (all the FIF files, Yokogawa, NIRS, Frontiers2018), and obtained the same results.
I will edit all the tutorials accordingly.

[ftadel]

Edited tutorials:

• https://neuroimage.usc.edu/brainstorm/Tutorials/StimDelays#Detection_of_the_button_responses
  I updated the screen capture and added @Moo-Marc's text below
• https://neuroimage.usc.edu/brainstorm/Tutorials/CorticomuscularCoherence#Event_markers
  I updated the screen captures and added a paragraph trying to explain why we set this value to 12 samples here.
• https://neuroimage.usc.edu/brainstorm/Tutorials/VisualSingle#Read_stimulus_triggers
• https://neuroimage.usc.edu/brainstorm/Tutorials/VisualSingleOrig#Read_stimulus_triggers
• https://neuroimage.usc.edu/brainstorm/Tutorials/NIRSFingerTapping#Extract_stimulation_events
• https://neuroimage.usc.edu/brainstorm/Tutorials/Yokogawa#Read_the_stimulation_information

Please have a look at the first two tutorials, and let me know if anything needs to be changed.

Thank you both for this thorough debugging!