Really view just one subprocessor in the LFP viewer, and also view events not corresponding to any data channels

[ethanbb]

This resolves #302. To summarize, the LFP viewer currently has the weird behavior of showing data from all subprocessors with a given index in their source nodes, rather than just one single subprocessor. This is convenient and works sometimes, but the incoming data from different sources could have different sample rates or different amounts of samples per buffer even with the same sample rate; dealing with this is complex and currently causes display glitches in some cases, and maybe instability.

This changes the combobox to let you choose a specific source node/subprocessor combination - for example, "File Reader 111/0" is the first subprocessor of the File Reader with node ID 111. Because there is now only one subprocessor being drawn, we only have to keep track of one current sample rate rather than an array of one per channel; however, I also added maps from subprocessor ID to sample rate and # of channels in the node, so that switching from one subproc to another works correctly.

As a side effect, we can now easily fix the issue brought up in #277 of Network Events TTLs not showing up, because we can use the information (i.e. # of samples) from the subprocessor that is being drawn to infer how to draw events with no associated continuous data. A simple way to do this is to treat any event channels with a sample rate of 0 as "standalone" events not corresponding to continuous data. I'm also making open-ephys-plugins/ZMQPlugins#2 that changes the Network Events event channels to have sample rate 0 so that this will work.

There are a bunch of changes, including some style things I did just to get rid of warnings in the process of working through the LFP Viewer code, so definitely ask if some parts don't look right/make sense!

[aacuevas]

Thanks for this!

The only thing I'm not convinced is using a 0 sample rate as indicator that the event is "standalone"
For this kind of events, the GUI sets timestamps based on a reference processor (by default the first continuous data source), using interpolation to approximate the moment of event generation (for example a command received by network) to their matching timestamp.
The same way, the event "sample rate" is set to that of the selected reference processor. This way, getting the timestamp is seconds is as easy as dividing timestamp by sample rate, and everything is contained inside the event info structure.

Using a zero-timestamp as a marker not only feels hackish, it also prevents having this information stored anywhere.

I'd advocate using a different approach. Maybe adding some fields to the Event class itself to register if the source processor had associated continuous data or not. Although I'm generally against adding stuff to the base classes, this could justify it.

[ethanbb]

@aacuevas OK, good point. I didn't realize the sample rate on event channels was used for that purpose.

What I had previously, before changing it at the last minute to setting the sample rate to 0 since I thought it was simpler, was that event channels with no source data would set all fields of the metadata source.channel.identifier.full to 0 (so that getChannelSourceID() would return 0). Maybe that's less hacky? Although the downside is it would be pretty tricky to discover for a new plugin author unless it was explicitly documented.

[aacuevas]

I was thinking on adding a field in Event telling if the source processor has continuous data or not.
A simple Event::sourceGeneratesTimestamps() (set to the source processor generatesTimestamps() ) could suffice.

It could also be useful to add some
Event::getTimestampSourceID() and Event::GetTimestampSourceSubprocessor()
methods, to get the exact source the timestamps are coming from.

[ethanbb]

OK, I'm not sure I see the full picture yet but it makes sense to me to make this information available in a less hacky way than in the metadata field. So do you want to just leave the PR open for now pending base class changes?

I'm going to revert the last commit that added the check for sample rate = 0, since it seems like we agree that was a bad idea.

[ethanbb]

I guess when you create the Event object, somehow the constructor would need to know the source node/subproc of the continuous data as well as of the event if those differ? (Instead of using the hacky metadata field.) So you could provide those as optional arguments, which would default to using the same as the event source which is known to the EventChannel? Except if the EventChannel source doesn't generate timestamps, in which case it would fall back to the current global timestamp source?

A little convoluted, but it would be flexible in that changes to the timestamp source or to the data channel backing an event channel would be reflected in any subsequently created events without making a new EventChannel object or needing to stop acquisition.

[aacuevas]

I left this for a while, sorry, but I believe it is time to revive it. I would want these changes to get into the next release I am preparing.
I updated a bit the EventChannel class from this discussion. There is a getTimestampsOrigin() method thar returns three possible values:

• timestampsFromContinuousSource, for those generated by a regular source node (like the Rhythm node)
• timestampsFromGlobalSource, for those generated by sources that do not generate timestamps (like the Network Events)
• timestampsDerivedFromChannel, for those that are created by a filter from a continuous channel (like the phase detector)
  For this last kind, there is also a dedicated constructor which, instead of a sampleRate has a DataChannel* as argument.
  I have also added two methods to get the sourceid and subprocessor which originates the timestamps (i.e., those of the channel). I am not sure if I should add also a channel idx field, so the metadata field would be redundant.

Could you please check the changes and update this PR accordingly if needed?

[ethanbb]

This is great, thanks! Working on updating the PR.

Is it me or are the new getter functions not yet implemented? I can't see them in InfoObjects.cpp.

[ethanbb]

@aacuevas: Should we still keep a separate TTL state for each subprocessor? As in, for this line:

ttlState[eventSourceNodeId] |= (1LL << eventChannel)

I'm not sure what should happen if the event is linked to the global timestamp source and not any specific data channel/subprocessor. It seems like having just one global TTL state might be less confusing. Is there a use case for maintaining separate TTL namespaces/states for different data sources?

[aacuevas]

Yes, the method bodies were missing from the commits, my bad. They are on the repo now.

Regarding your second question, the truth is that the LFP viewers still rely on old core, before event channel objects were a thing. That is also why they only support 8 TTLs with no distinction of source.
I did some additions in preparation for eventual improvements that made such distinctions, but they haven't happened yet.

[ethanbb]

OK, so I guess TTL events not linked to a specific data channel could alter the TTL state of all sources? Does that seem logical?

[ethanbb]

Or if you really wanted to restructure things, another way would be to make Subprocessor an actual class that holds the sample rate (if it has its own), a TTL state, the new fields you just added and maybe some other information, and can be linked to any combination of data and event channels. Then you could say that a plugin like Network Events has one Subprocessor, which is linked to an event channel but no data channels. And all or any combination of TTL states from incoming subprocessors could be displayed in the LFP viewer, using the sample rate ratio between subprocessors if necessary to transform the start and end times so they align correctly.

Maybe not what you want to do since subprocessors are still being phased out, right? Or maybe "streams" will work something like this?

[aacuevas]

Do you mean throughout the system or just for the LFP viewer? For the whole system we have to take into considerations the limitations of the JUCE library, as well as make it general enough. "ttl states" is only relevant to visualization, as TTL events have value on their own. Mixing them for visualization is actually a non desirable side effect of the viewers being based on an old architecture. Ideally we should be able to select which TTL events to visualize. But that is an extensive patch outside of the scope of this PR.

For the sake of getting this PR working, I have added a new CoreServices method, getGlobalTimestampSourceFullId(), which I believe has enough merit to exist. I have also updated the event channel objects so the timestamp origin is set to that same value when timestampFromGlobalSource.

Since for visualization we are, indeed, mixing all ttl events with a shared timestamp source into a single state, for events with timestampsFromGlobalSource we can now just use the same method for all three types and set the joint TTL state for each source.

I would like this PR to be integrated into development as soon as possible, as we are planning a new release soon and I feel this should be into it.

Thanks for all the help!

[ethanbb]

Do you mean throughout the system or just for the LFP viewer?

I was talking about the whole system, but also I was just trying to throw out more ideas because I wasn't sure if the silence for a couple of days meant you were stuck/confused about the issue I was asking about. But I now see that I had some misconceptions....

"ttl states" is only relevant to visualization, as TTL events have value on their own. Mixing them for visualization is actually a non desirable side effect of the viewers being based on an old architecture. Ideally we should be able to select which TTL events to visualize. But that is an extensive patch outside of the scope of this PR.

OK, interesting, I didn't realize this.

So regardless of whether they are grouped or independent, my main issue was that giving each subprocessor its own set of TTL channels that don't interact with each other seemed needlessly complex and I couldn't figure out how a generic TTL input not associated with any particular data would fit into that framework. On the other hand, I realize that you designed it that way on purpose and I'm not really trying to suggest this PR should trigger a big architectural change and reversing that design decision.

Allowing a non-data-linked TTL event to operate on the TTL channels of the global sample rate source (in preference to any other sources) still doesn't make complete sense to me, but I can implement it that way with the help of the new method you added (thank you).

-Ethan

[aacuevas]

I do believe there is amisconception with ttls and data links.
Most of the TTLs are actually not data linked. Any TTL inputted into the acquisition board, for example, is not inherently linked to any data. It is just that the timestamps are generated by the acquisition board.
An event generated by a filter is usually linked to a channel. But keep in mind that a filter might generate other kind of events. There could be, for example, a processor that detected peaks and generated a binary event with the amplitude for any local peak. TTLs are just one particular type of event. Of course, those events timings are related to a channel and share timestamps with the processor which created the channels.

And now we get to the third type. Stuff like the network processor creates events (which may be TTL, text messages or binary events) with no relation to any source whatsoever. But when recording (which is the priority) we need to be aware of when they were generated. That is where the global timestamps come into action. Global timestamps are actually "timestamps for processors which do not produce their own but produce events", nothing more.

By default they are interpolated from the first continuous source, but it can be selected with Edit/Timestamp source.

For recording, there no issue at all. Everything get recorded with their timestamps that can, in turn, be converted to seconds, synchronized with other sources, etc... Keep in mind that the priority is always recording, and having all the data present in the recordings.
Visualizing is another issue. For example, one might want to see events in the LFP viewer generated by the network events module, even when they are not related to the continuous channels being shown.

Now, the LFP viewer on itself is in an incomplete state related to that. Right now it just lets you see "8 channels" which originally corresponded to the hardware TTLs. In a ideal scenario, there would be 8 (or N) visualization "channels" whose actual event source could be selected.
Also, the way it works now, trying to synchronize events and data coming at different rates with different timestamps does not work properly. That why in the LFP viewer we have now we added the selector, so it only shows continuous data related to a single source. Ideally, we should maybe do that the visualized events are also those whose timestamps are related to that source, that should not be too difficult, but is still some work outside the scope of this PR.

[ethanbb]

Thanks for explaining your perspective on this in detail. I think it all makes sense. So if I understand correctly, you'd rather keep every single TTL event channel's state separate and individually visualizable, but because of the limitations we have now, we're going to combine all channels with a common timestamp source into one state?

Ideally, we should maybe do that the visualized events are also those whose timestamps are related to that source, that should not be too difficult, but is still some work outside the scope of this PR.

I believe this will actually be how it works in this PR. I should be able to finish it tonight.

[ethanbb]

OK, should be working now. See also open-ephys-plugins/ZMQPlugins#4.

[aacuevas]

Great, thanks!