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

Postsynaptic model target #458

Merged
merged 4 commits into from
Sep 15, 2021
Merged

Conversation

neworderofjamie
Copy link
Contributor

@neworderofjamie neworderofjamie commented Sep 15, 2021

As described in #451, previously, you needed to define a new postsynaptic model for each additional input variable you might want to connect synapses to e.g. you should be able to use a standard DeltaCurr rather than having to define:

feedback_psm_model = genn_model.create_custom_postsynaptic_class(
    "feedback_psm",
    apply_input_code="""
    $(ISynFeedback) += $(inSyn);
    $(inSyn) = 0;
    """)

just to connect synapses to a neuron's ISynFeedback additional input variable. This PR instead allows you to do this in C++:

auto *syn= model.addSynapsePopulation<WeightUpdateModels::StaticPulse, PostsynapticModels::DeltaCurr>(
    "Syn", SynapseMatrixType::DENSE_INDIVIDUALG, NO_DELAY,
    "Pre", "Post",
    {}, { 1.0 },
    {}, {});
syn->setPSOutputVar("ISynFeedback");

or in Python

syn = model.add_synapse_population("Syn", "DENSE_INDIVIDUALG", NO_DELAY,
    pre_pop, post_pop,
    "StaticPulse", {}, {"g": 1.0}, {}, {},
    "DeltaCurr", {}, {})
syn.ps_output_var = "ISynFeedback"

The implementation is trivial, my only question is whether "PS output variable" is a good names for these things rather than, for instance, "Target input variable" or "Postsynaptic input variable". Any thoughts much appreciated!

Fixes #451

…`` implementation

* Include the PS output var in hashes
* Substitute $(Isyn) for PS output var in postsynaptic update substitutions
@neworderofjamie neworderofjamie added this to the GeNN 4.6.0 milestone Sep 15, 2021
@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

Merging #458 (2a6d482) into master (967ffba) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
+ Coverage   87.21%   87.23%   +0.01%     
==========================================
  Files          78       78              
  Lines       17038    17057      +19     
==========================================
+ Hits        14860    14879      +19     
  Misses       2178     2178              
Impacted Files Coverage Δ
include/genn/genn/synapseGroup.h 90.90% <100.00%> (+0.21%) ⬆️
...c/genn/genn/code_generator/generateNeuronUpdate.cc 95.66% <100.00%> (+0.02%) ⬆️
src/genn/genn/synapseGroup.cc 81.88% <100.00%> (+0.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 967ffba...2a6d482. Read the comment docs.

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.

Yes, all good but could consider a different name. I think input/output maybe not ideal concepts as one's input is the other one's output.
How about "PSTargetVar"?
I think you have used "PS" for post-synaptic before so is in good keeping with that practice.

@neworderofjamie
Copy link
Contributor Author

Excellent suggestion!

@neworderofjamie neworderofjamie merged commit b6a4f56 into master Sep 15, 2021
@neworderofjamie neworderofjamie deleted the postsynaptic_model_target branch September 15, 2021 15:01
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.

Make pointing synapse groups at additional input variables easier
2 participants