recordEngine - improve writeTimestampSyncText signature

[d1manson]

The old version had a few quirks:

1. Provides streamId, but there is no way to lookup a stream by streamId within the record engine, only by index.
2. Provides 0 for the sourceSampleRate (this is hardcoded at the one call site), so this param is useless.
3. Despite being called "..Timestamp.." it provides the sampleNumber rather than a timestamp. This is also especially painful as the other virtual functions on the record engine recieve timestamps not sample numbers, and I don't think you have access to the synchroniser to do the conversion in either direction.

So to address that, I suggest:

• A. Provide a DataStream* rather than a streamId, this means you can also get any properties you want from the stream, such as the sample rate. A nullptr can be provided for this arg if needed, and indeed it seems it is (I don't quite understand how/why that is happening, but the writeTimestampSyncText function has historically been called with a non-stream first).
• B. Provide the timestamp as well as the sampleNumber. This timestamp should match up to the timestamps provided to the other vitual functions.

This is of course a breaking change to the plugin api. But it should be pretty trivial to migrate given you can always call stream == nullptr ? 0 : stream->getStreamId() to recover the streamId, and aside from that we haven't lost anything (given the sourceSampleRate was never providing any actual data, and the new timestamp arg is an addition.

In terms of thread-saftey considerations, as far as I understand I'm not doing anything especially novel in the RecordThread, but worth someone who understands things better checking this.

In terms of the timestamp calculation, it would be good for someone to confirm this is right. I adapted the logic from the call to dataQueue->writeSynchronizedTimestamps(..) within RecordNode::process.

--

In addition to the main change here, I've also added a getDataStreams() function on the RecordEngine, wrapping the function on the underlying recordNode.

Without that there was no way to iterate over the streams safely, as far as I can tell. You might think you can iterate from 0 to getNumRecordedDataStreams(), and call getDataStream(index) in each case, but that doesn't work because getNumRecordedDataStreams can return a number less than the total number of streams, and there's no reason that the recorded stream(s) are guaranteeed to come first in the list (i.e. if you stop iterating too soon you might miss actual recorded streams).

TODO:

• Update PLUGIN_API_VER, presumably.

I have posted about this in discord here.

[anjaldoshi]

Hi @d1manson,

Thanks for the detailed PR and explanation! Let me provide some context about the writeTimestampSyncText method that might help clarify its purpose and history.

First of all, the writeTimestampSyncText method was added as a backup because there was a bug in an earlier version of KiloSort that was deleting all files in the recording directory, causing the sample_numbers.npy to get deleted. This method is called once per incoming data stream at the start of each recording in order to inform the Record Engine about the first sample number recorded by each data stream, which is then used by the Binary Format engine to create the sync_messages.txt file. This was used as a way to recover the sample numbers in case the sample_numbers.npy gets deleted. Writing these values to disk is optional and is not needed anymore (since GUI v0.6.0), especially considering the streams are already synchronized online.

The reason why the method is named writeTimestampSyncText and not writeSampleNumberSyncText is due to the naming convention used in GUI versions prior to v0.6.0 (samples numbers were saved as timestamps).

I'm not very sure about the motive behind making these changes. Are you writing a record engine that needs to write the first sample number AND timestamp at the start of each recording? If so, why can't you use the timestamps and sample numbers that are being written via the data-specific write methods (writeContinuousData, writeEvent, and writeSpike)?

Regarding the PLUGIN API version bump: we only increase the API version as part of a planned major GUI release (e.g., v2.0.0) that delivers a cohesive set of breaking changes and/or substantial new features. This change doesn’t meet that bar, and bumping early would create extra maintenance for downstream plugins (rebuilding and redeploying) and cause existing plugins to fail to load after updating the GUI.

If you could provide some more clarity on your use case and what specific functionality you're trying to achieve, that would be great. Understanding the motivation will help us better evaluate whether these API changes are necessary or if there's an alternative approach that would work for your needs.

Thanks!

[d1manson]

Thanks @anjaldoshi for the quick reply here.

In terms of why I need this (at least why i think i need it)...

At least prior to v1 (and I think this remains the case), if you want the zero time to correspond to the start of recording rather than the start of acquisition, you needed to listen for the writeTimestampSyncText* and then within the other write* methods you need to (a) subtract off the recording start time from all timestamps (see here); and (b) discard any data from before the start of the recording (see here).

In terms of why I only suggested this now, the main reason is that whereas before there was only one data stream from the aquisition board, now there are two (the memory_usage is the second), which complicates a few things downstream where preivously I was able to ignore the possibility of additional datastreams.

For now I have built the main GUI app on my fork (see here), so that it is available for users of this plugin, but in the longer run it would be nice if this PR did make it into the main repo, assuming it does make sense to do so.

*see here for the version that doesn't rely on this change to the plugin api, or here for the version that works with this change to the api.

[d1manson]

In terms of the wider context for why i have this plugin (actually a pair of plugins), they are trying to recreate the kind of data files and workflow that are familiar to several people at UCL (and others in Europe who have been using the same hardware/software for a few decades). For now it's only being used by Guifen Chen's lab at QMUL (she was at UCL previously), but might be of interest to others who have come up with alternative shims to migrate to OE without having to make big changes to their workflow and matlab code etc. (I know of at least one person/lab in that category).

[d1manson]

Not exactly related to this, but I believe i've encountered a bug where getNumRecordedDataStreams returns 0 rather than 1. Acording to a search on github no one is currently using the function anywhere, so maybe it really is a bug.

Not sure what is causing it to return 0, but it is now consistenly doing so.