Reviving PSTH as a plugin

[shayo]

Hi Aaron,

I've made some changes to try and revive PSTH. I haven't tested things fully yet (this is work in progress), but I thought I should commit this in the meanwhile to get feedback.

[aacuevas]

Awesome, thanks! We were having trouble getting time to finally try to port this processor to the plugin architecture, so it's great to have you on board with this.

Seems mostly good, but looking at the commit log there a couple of aspects that I fear might not be much future-proof (we don't want this to stop working again next time we change anything ;) ) or that might cause trouble in some corner situations. I'm going to download this before merging so I can get a good look at it in a real code editor. Give me a couple of days and I'll give you detailed feedback.

[shayo]

The one thing I would like to see is two-way inter-plugin communication. Not sure what is your thoughts on that. Right now I've used the the existing message passing and restricted communication to one way (spikesporter->psth).

[aacuevas]

That's quite a difficult topic. Our current approach is to encourage isolated and interoperable plugin design, so no plugin requires of other specific plugin to work properly. They might require a function up the chain (like spike detection), but any plugin that fulfills that function should work. No plugin should be hard dependent on any other and, all plugins should only need ro know what's coming to them, doing their work without having to worry what's ahead of them.

Thus our current thinking is that the best way to attain this is with the kind of "inter-plugin communication" we have now (although it can a will be improved): messages sent down the chain, so processors can inform others down the chain of "all they need to work", the less processor-specific the messages, the better.

We do recognize that in some specific cases, a way to communicate data upstream might be needed (the best example of this is sending data to the outputs of an acquisition board), so we are thinking of ways to achieve this that do not require the use of raw pointers to processors or other low level stuff, probably using some kind of message passing. But we're not there yet.

[jvoigts]

Yea this is a topic we should think through properly some time soonish. My gut feeling is that we could likely come up with a good extension of the message passing. As to the upstream communication, i think this would be somewhat solved once we have the proper channel subset handling and UI in place: The user would then just select the 'message passing channel' coming out of the downstream plugin and loop it back into the upstream plugin, possibly into a specific input interface. This would not solve the interchangeability concern, but at least it would be clear to the user what is going on, and we would have an explicit representation of the whole data flow in the config etc.

[aacuevas]

At the moment, I have only two big concerns with the code.
One is regarding the StringTS class. You pass them as "MESSAGE" events, but those are, by definition, zero-terminated ascii strings which must be recorded into a human-readable format, while a StringTS is passed as the string with a binary timestamp attached at the end. I'd recommend using "BINARY_MSG" events instead, which allow you te send any binary blob down the chain. If you want some of those messages to be logged then you can just send a parallel text-only MESSAGE event, that will be recorded.

The other is the use of "common" plugin files. While, in principle, it seems like a good idea when two plugins use common structures, we discovered that the Linux dynamic linker complained when two plugins have identical named symbols (which is a result of reusing source files). In fact, I was about to add a plugin design guideline stating that all the code for a plugin should be contained into its own namespace, to avoid these conflicts. (Although this can be solved for common files using a macro to define a namespace, so it's possible to reuse the same code with different namespaces).

Other than that, all else are nitpicks related with an effort to adopt proper Juce coding standards, such as using HeapBlocks instead of simple arrays (thus avoiding calls like "new int[xx]" and having to delete them afterwards) or using ScopedLocks instead of the enter and exit methods on mutexes, as explained here. (BTW, are all mutexes necessary? I can't see that many concurrency)

[shayo]

Hi Aaron,

1. I wasn't aware of the new BINARY_MSG event. Sure. That makes more sense. I can change that.
2. The common folder is suppose to remove direct dependency across modules. There is already code shared across all plugsins (i.e., code to handle events). Not reusing code seems wasteful and a source for future bugs. I advise against this. There is a difference between asking modules not to depend on other modules, and not to depend on common infrastructure. The latter doesn't make sense to me.
3. I agree that adhering to juce code standards would be better - however, this is certainly not high on my priority at the moment.

Shay

[aacuevas]

The issue with 2 is not common code, which I agree with you, it's how things should be done, but with symbol name duplicity: if two different modules have a symbol with the same full name, the Linux linker will refuse to load one of them.
In case of code from the GUI (Juce code, GenericProcessor methods, etc...) those methods are not built in the plugins at all, but on the Core GUI, who then exports them for plugins to use. So they exist only once in the whole process space.
When building plugins, however, StringTS (for example) gets compiled into the NetworkEvents plugin, the SpikeSorter plugin and the PSTH plugin separately, existing three different copies of it. This causes issues in some cases. If there were only one binary source for those methods, or if they were named differently (for example, using namespaces: NetworkEventsPlugin::StringTS::StringTS() is different from PSTHPlugin::StringTS::StringTS() ) there would be no issue.

So, first of all, demanding that all the code from a plugin is enclosed in its own namespace is to avoid general symbol conflict (imagine the case where 2 different plugins implement a class with the same name but which behaves completely differently, this would avoid issues with that).

Now, to solve this for common code, there are three solutions: The bad, the good and the (quick and) ugly 😆 :

• The Bad is just copying the same code into each plugin with a different namespace. Bad solution, bug prone, redundant effort... Name your poison, this will have it.
• The Good would be encapsulating common classes into their own libraries (create a StringTS.dll, a ContinuousCircularBuffer.dll, or a dll containing different but related classes). This would enable different plugins to reuse the same code, and it would exist just once in the process space. It's the one that offers most advantages. The only downside is that, well, we have to develop yet another dynamic lib with all the hassle it supposes.
• The (Quick and) Ugly would be to use a namespace tied to a macro in the common files (something like namespace NAMESPACE_NAME {}, while defining NAMESPACE_NAME in the project. This way the code will be reused, although it would be compiled multiple times (like it does now), but with no symbol name collision.

At least, there are the only ones I can think of right now.

[ethanbb]

Hi Aaron,

Any new thoughts on sharing libraries between plugins? One of the plugins I'm working on will probably need to use the Dsp library currently used in the FilterNode, and I'm wondering if it can be put into a shared library or something to avoid duplication. Currently it looks like the FilterNode compiles its code and the library functions it uses into the same object file.

(Sorry this isn't on topic with the pull request.)

[beOn]

Pretty sure there's some overlap with issue #89, if you're looking for a place to continue the conversation about 'common files.'

[ethanbb]

@beOn, that is a pretty nasty-looking bug, but I'm not sure it's directly related to my question which is basically a feature request regarding a space for common libraries for plugins. It is a good example of why we need to be careful with this kind of code-sharing situation, though. I can open up a separate issue.

[beOn]

Ah, my thought was that they’re related in that the other bug involves sharing the juce framework. We’ll run into similar situations if multiple plugins are using the same library, and that library includes singletons.

I’ll follow you to the separate thread :).

Ben

On Sep 6, 2016, at 1:14 PM, Ethan Blackwood notifications@github.com wrote:

@beOn https://github.com/beOn, that is a pretty nasty-looking bug, but I'm not sure it's directly related to my question which is basically a feature request regarding a space for common libraries for plugins. It is a good example of why we need to be careful with this kind of code-sharing situation, though. I can open up a separate issue.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #57 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAC-ROUo6ysptM7Na3tlWo_C_a02fljkks5qna2cgaJpZM4I3aS_.

[aacuevas]

Reviving this topic a bit, I've been creating a way to put those common classes on library files, you can find the details on #90 . @shayo I definitely think this should be the way to go for those shared classes, so there is no duplicity of code either on the source side neither in the binary form. What do you think?

[shayo]

Hi Aaron,

Compiling shared code into a separate library sounds reasonable.
Do you have an example project implementing this?

[aacuevas]

Yup, both the KWIK and NWB plugins on the NWB branch of my repo use the now called OpenEphysHDF5Lib which includes some shared classes. It still works only on Linux and Windows, we need to figure out how to make those in OSX, but I imagine that's just a matter of time.

[shayo]

OK. I'll take a look at them when I need to do ephys again. Hopefully, by then they will be merged to the main branch.

[beOn]

@shayo, @aacuevas, does someone need to take over finishing the PSTH changes so we can merge it back into the development branch? If so, I volunteer. Please let me know soon.

Or does this request just need to be re-directed to the development branch and accepted?

Either way, let's get a move on. My stimuli aren't gonna peri time histogram themselves!

[beOn]

Okay, put together a branch that adds an xcode plugin project and gets PSTH working. It still uses the 'common' directory, though.

[shayo]

Sorry Ben, I'm terribly busy these days with other stuff. You or Aaron will have to work your magic to get it up & running again.

…

-- Shay Shay Ohayon HHMI Postdoc Fellow of the Life Sciences Research Foundation DiCarlo's lab, MIT, 46-6147 +1 617-324-3593

On Mon, Nov 28, 2016 at 3:41 PM, Ben Acland ***@***.***> wrote: Okay, put together a branch that adds an xcode plugin project and gets PSTH working. It still uses the 'common' directory, though. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#57 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4tDt5qOuozG-ygQya8nRrnKntwE1-7ks5rCzyDgaJpZM4I3aS_> .

[beOn]

No worries! I'll take point. @aacuevas, unless you have some other advice, I'll use the approach from #90 for common code.

[aacuevas]

@beOn I've merged into the development branch the code in my personal copy linked in #90 so you can start using the common library architecture easily.

As a note, we're in the process of redesigning the whole "channel" structures as well as the ways processors have to send data downstream, since the current code is becoming difficult to maintain and to expand by plugins. It might be interesting to have this into account since the Spike Sorter and the PSTH make heavy use of those systems so it could be good to have the new structures in mind so we don't have to redesign the whole plugins as soon as they are finished.

The code is currently unfinished, with the plan of being finished and all the plugins updated by January. Since it is still a work in progress there is no good documentation on it yet, but you can get an idea of what form the new classes will have by taking a look at the header files here and here.

I cannot guarantee that the current prototypes are going to remain unchanged, but the basic ideas will be the same. Feel free to ask me about anything related to this changes so we can work together and avoid having to recode twice the same.

[beOn]

Cool deal - using the new channel structures sounds good. I'm a little slammed this week, but'll take a look at the headers and ask you whatever questions arise before diving in.

[jsiegle]

PSTH functionality is now available in the Event Triggered Average plugin, as well as by streaming to other processing, such as OPETH.