Use 2-triggers to account for EEG-sound card clock drift.

[tomstoll]

I made an attempt at inserting 2-triggers so we can use them to estimate/correct clock drift. Currently, the default behavior is 'end' which inserts a single 2-trigger at the very end of the trial. It can also accept a single time (in seconds) or a list of times when the 2-triggers should be stamped. Positive values will be the time from start of trial and negative values will be time from end of trial. I think an empty list would result in no 2-triggers being stamped. It still needs error handling to make sure triggers don't overlap or occur outside the trial time.

[tomstoll]

Tried to implement the above suggestion and add some warnings in case triggers overlap or are outside the stimulus window. We also thought it would be good to use the bitwise or operation instead of adding the triggers in case any overlap, but because the triggers are floats by this point I wasn't able to do that. Any thoughts on this @larsoner?

Forgot to mention, I also moved the code inside the if self._n_channels_stim > 0: loop. But looking at the earlier code, there's an assertion assert self._n_channels_stim >= 0, so do we still need these checks? There's a few of them throughout the script.

[codecov[bot]]

Codecov Report

Merging #424 (3e2358d) into main (b6abe98) will decrease coverage by 0.87%. The diff coverage is 53.48%.

:exclamation: Current head 3e2358d differs from pull request most recent head 5713351. Consider uploading reports for the commit 5713351 to get more accurate results

@@            Coverage Diff             @@
##             main     #424      +/-   ##
==========================================
- Coverage   87.97%   87.10%   -0.88%     
==========================================
  Files          50       49       -1     
  Lines        6829     6708     -121     
  Branches     1099     1110      +11     
==========================================
- Hits         6008     5843     -165     
- Misses        558      590      +32     
- Partials      263      275      +12     

[larsoner]

We also thought it would be good to use the bitwise or operation instead of adding the triggers in case any overlap, but because the triggers are floats by this point I wasn't able to do that. Any thoughts on this @larsoner?

We could make _make_digital_trigger return int32 data, and add a new _scale_digital_trigger that does the ((trigs << 8) * self._trig_scale).astype(np.float32) step. Then you can add in bit-perfect-int-land and scale afterward.

[rkmaddox]

That's exactly the solution we discussed as well.

[tomstoll]

Gave it a shot. I removed the lines that bitshift and convert to floats from _make_digital_trigger and changed from inserting the 2-triggers in samples to inserting them in stim. Then I added the _scale_digital_trigger function to do what was removed from _make_digital_trigger and used that on stim after using np.bitwise_or to add in the 2-triggers. I also scale the triggers in stamp_triggers immediately after the call to _make_digital_trigger.

I also updated the warnings so that it will skip the triggers that occur outside the stimulus window and prevent an error, and checking for the overlapping 2-triggers actually works (previously always gave the warning because I forgot the <trig2_len part in the if statement).

[larsoner]

Failure looks like it might be real and related:

expyfun\tests\test_experiment_controller.py:616: in test_sound_card_triggering
    ec.identify_trial(ttl_id=[1, 0], ec_id='')
expyfun\_experiment_controller.py:2038: in identify_trial
    self._id_call_dict[key](id_)
expyfun\_experiment_controller.py:2080: in _stamp_binary_id
    self._stamp_ttl_triggers(id_, wait_for_last, True)
expyfun\_experiment_controller.py:2121: in _stamp_ttl_triggers
    ids, wait_for_last=wait_for_last, is_trial_id=is_trial_id)
expyfun\_sound_controllers\_sound_controller.py:349: in stamp_triggers
    stim, ((0, 0), (0, self._n_channels)[self._stim_sl]), 'constant')
<__array_function__ internals>:6: in pad
    ???
c:\python37-x64\lib\site-packages\numpy\lib\arraypad.py:743: in pad
    pad_width = _as_pairs(pad_width, array.ndim, as_index=True)
c:\python37-x64\lib\site-packages\numpy\lib\arraypad.py:518: in _as_pairs
    return np.broadcast_to(x, (ndim, 2)).tolist()
<__array_function__ internals>:6: in broadcast_to
    ???
c:\python37-x64\lib\site-packages\numpy\lib\stride_tricks.py:411: in broadcast_to
    return _broadcast_to(array, shape, subok=subok, readonly=True)
c:\python37-x64\lib\site-packages\numpy\lib\stride_tricks.py:350: in _broadcast_to
    op_flags=['readonly'], itershape=shape, order='C')
E   ValueError: operands could not be broadcast together with remapped shapes [original->remapped]: (2,2)  and requested shape 

[tomstoll]

Yup, been looking for it for a while. Think I got it. I wrote triggers where I should have had stim.

[larsoner]

Now that #425 is merged, I did a merge with upstream/main -- your code was not tested on Linux or macOS because we ran out of Travis credits :(

Will merge assuming CIs come back happy, thanks @tomstoll !

[larsoner]

... and I also pushed a tiny commit.

Just noticed that it's still entitled WIP -- did you want to check to make sure everything works locally or anything else, or is this good to go from your end?

[larsoner]

53.48% of diff hit (target 95.00%)

This is not great, however -- maybe worth adding some tiny test that actually uses this feature?

[tomstoll]

Just a couple more things to do on our end. We think it would be nice to put the samples when the 2-triggers occur in the tab file so they're easier to use during analysis. Looking at codecov, some of the changes in coverage it reports don't seem to be from the changes here, which I'm a little confused by.

[larsoner]

Codecov is a little bit flakey, I would just concentrate on the changes here. For logging indeed it seems reasonable to log the relative times that will be stamped for each stimulus, probably just the list of float of times actually used would be good ([1., 2., 3., 3.245] or whatever).

[tomstoll]

The times that 2-triggers are stamped will now be written to the tab file. I also tried to add some tests, so we'll see how that goes.

[larsoner]

I'd just push

[larsoner]

Awesome, thanks @tomstoll !

[tomstoll]

Great, thanks for your help! This should close issue #421.