Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Step 6 - Fused event generation and postsynaptic spike-like events #609

Merged
merged 119 commits into from
Jan 31, 2024

Conversation

neworderofjamie
Copy link
Contributor

@neworderofjamie neworderofjamie commented Dec 4, 2023

One feature that has been notably missing from GeNN is postsynaptic spike-like events. These would be very useful for implementing all kinds of event-based learning rules e.g. to add per-MBON event-based dopamine signals to a mushroom body model. However, the spike-like event system was a bit delicate (see #379 and #594) and definitely couldn't take any more complexity!

This PR adds in this functionality and lays the ground-work for enabling the "spike-splitting" optimisation described in #484 which would hugely improve performance of convolutions. Essentially what this PR does is:

  • Remove the (mostly commented out until now in GeNN 5) system for figuring out per-neuron group spike-event thresholds and the threshold re-retesting mechanism in synapse updates (this bit was always problematic)
  • Change spk and spkCnt from being per-neuron group to per-fused synapse group like inSyn etc if the appropriate option is enabled (for spikes it always is), reusing the same mechanism used for Merging of compatible postsynaptic models #201 and Merge pre and postsynaptic update #461 along with the more elegant merged group implementation from Step 1 - Transpiler #595.

However, as ever, the devil is in the details and fusing these synapse groups is rather trickier than e.g. a postsynaptic update because, the neuron code doesn't care whether spike/spike-event thresholds are presynaptic or postsynaptic and should fuse them regardless. However this means that one synapse groups presynaptic end might be fused to another's postsynaptic end if they correspond to the same neuron group. The bugs this caused have been...fun...hence the additional logging functionality I've added to Runtime which should now tell you everything reading the source of runner.cc would have 😄

* Because ``NeuronGroup`` will merge spike conditions from incoming and outgoing synapses, need to make fusing-related ``SynapseGroup`` figure out whether they're pre or postsynaptic based on NeuronGroup. As a bonus, this reduced a lot of WUM duplicate code
* Started adding data structures for spikes to SynapseGroup and NeuronGroup
…to ``SynapseGroup``"

This reverts commit 3bf3139.

# Conflicts:
#	src/genn/genn/code_generator/neuronUpdateGroupMerged.cc
Copy link
Member

@tnowotny tnowotny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like something that should be sorted but could be endlessly tricky. Are you confident you have covered all nasty confusions?

Base automatically changed from dynamic_parameters to genn_5 January 9, 2024 10:06
@neworderofjamie neworderofjamie mentioned this pull request Jan 10, 2024
@neworderofjamie
Copy link
Contributor Author

It is definitely tricky - I've been fixing bugs in the Final PR (#613) based on new tests/STDP models. However, I think it's the right thing to do and necessary as it lays the groundwork both for learning convolutions efficiently and more advanced event-driven three-factor learning.

Copy link
Member

@tnowotny tnowotny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved though detailed code review at this scale is beyond what I can do.

@neworderofjamie neworderofjamie merged commit c2dc7a3 into genn_5 Jan 31, 2024
1 check passed
@neworderofjamie neworderofjamie deleted the event_merging branch January 31, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants