Fix issue: #3532: Disconnect correctly with CompressedSpikes:ON/Off

[med-ayssar]

Fix for #3532
The main issue occurs when CompressedSpikes is disabled — the wrong connection gets deleted. The following image illustrates the problem."

Issues:

• With compressed-spikes off, std::lower_bound was being applied on unsorted vector.

• With compressed-spikes off, and regardless of (1), disconnecting a connection was not implemented correctly, because the query on the snode_id and tnode_id delivers a wrong lcid.

• While testing, I discovered that disconnecting a connection, it changes the entry in the sourceTable, and when working with a sorted sourceTable (compressed-spikes=true), we might get an undefined behaviour

[heplesser]

@med-ayssar Thanks for your detective work and the PR! I will look at it in detail asap.

[heplesser]

@med-ayssar Thanks for the PR! I am not entirely sure I follow the logic around nth_equal(), see comments below.

I also wonder if, for the uncompressed case, we should not take a different approach. In that case, I believe, we simply need to run through all connections of a given synapse type linearly and disconnect the first one for which source and target match, and which has not yet been disconnected. I guess that means parallel iteration through source and target tables. The split into find_first_source() and find_first_target() does not make sense in that case.

We also need some tests, including tests where there are multiple connections between the same source-target pair and we delete one at a time.

[med-ayssar]

@heplesser: Is it possible to setup a meeting to discuss this? Any time after 17:30 should work for me.

[heplesser]

Hi @med-ayssar! Thanks, this looks very good now. I will shortly send you a small PR which removes a few unused imports and an unused method, and adds/improves doxygen comments. Once that is merged, I will approve.

[heplesser]

@med-ayssar Thanks, all fine now from my side.

[heplesser]

@cjotade Thank you for your review! e-prop synapses hold a pointer to an optimizer object (could be simple gradient descent or an Adam optimizer). This optimizer needs to be destroyed when the synapse is deleted. The old code contained a bug that would lead to NEST trying to destroy the optimizer twice, and trying to delete an already deleted object leads to a runtime error. The changes make sure that the optimizer is deleted exactly once. And the change is in this PR because I discovered this problem when testing disconnection with all our synpase types, including eprop synapses.

[cjotade]

I see, thanks for the clarification!

[heplesser]

Adding empty lines to fulfill coding standards.

[heplesser]

@cjotade With your test now merged, could you re-check and then approve explicitly if you think the PR is ready to be merged?

[cjotade]

Everything correct from my side, I think that the PR is ready to be merged. Thanks!