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

Add to pre #479

Merged
merged 23 commits into from
Nov 18, 2021
Merged

Add to pre #479

merged 23 commits into from
Nov 18, 2021

Conversation

tnowotny
Copy link
Member

@tnowotny tnowotny commented Nov 12, 2021

This is the PR for adding additional functionality of
$(addToPre, XXX) to synapses sim_code, synapse_dynamics_code, event_code, and post_learn_code.
The expression is added to the presynaptic neuron's Isyn by default but the target variable can be set to a different input variable of the presynaptic population.

This is still work in progress in the sense that there should be additional feature tests, at least testing for the use in event code and post_learn_code in addition to synapse_dynamics and sim_code that are covered by the two tests I created.
One could also add a set of tests for other connectivity schemes than dense and for the different parallelisation strategies to get a good coverage ...

Developer notes:
I re-used the fusePSM flag to decide whether the presynaptic synaptic output should be fused, and did not introduced a new, separate flag.

I am using the same HashDigest for everything as there are no parameters, i.e. not separate fuse has digest.

The collection variable for reverse presynaptic output is "revInSyn".

I am re-using getInSynLocation, i.e. locations will be identical for inSyn and revInSyn.

I am also allowing $addToPre in synapse_dynamics code.

I did not use shared memory for the SIMT code and PostSpan, PostSpanBitmask strategies. I am not sure whether it would be worth it and also do not want to overload shared memory with yet another separate memory structure.

TODO

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #479 (e16360d) into master (33b81dc) will increase coverage by 0.04%.
The diff coverage is 91.93%.

❗ Current head e16360d differs from pull request most recent head 9186f3e. Consider uploading reports for the commit 9186f3e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #479      +/-   ##
==========================================
+ Coverage   87.46%   87.51%   +0.04%     
==========================================
  Files          82       82              
  Lines       17266    17437     +171     
==========================================
+ Hits        15102    15260     +158     
- Misses       2164     2177      +13     
Impacted Files Coverage Δ
include/genn/genn/neuronGroupInternal.h 0.00% <ø> (ø)
include/genn/genn/synapseGroupInternal.h 75.00% <ø> (ø)
src/genn/backends/single_threaded_cpu/backend.cc 57.12% <66.66%> (+0.15%) ⬆️
...nn/code_generator/presynapticUpdateStrategySIMT.cc 89.32% <70.58%> (-1.13%) ⬇️
src/genn/genn/synapseGroup.cc 85.77% <95.55%> (+0.62%) ⬆️
include/genn/genn/code_generator/groupMerged.h 84.29% <100.00%> (+0.10%) ⬆️
include/genn/genn/neuronGroup.h 74.13% <100.00%> (+0.45%) ⬆️
include/genn/genn/synapseGroup.h 92.30% <100.00%> (+0.47%) ⬆️
src/genn/genn/code_generator/backendSIMT.cc 96.60% <100.00%> (+0.01%) ⬆️
src/genn/genn/code_generator/generateRunner.cc 95.98% <100.00%> (+0.01%) ⬆️
... and 4 more

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 33b81dc...9186f3e. Read the comment docs.

@@ -0,0 +1,20 @@
CXXFLAGS +=-std=c++11 -Wall -Wpedantic -Wextra -I $(GTEST_DIR) -isystem $(GTEST_DIR)/include
Copy link
Contributor

Choose a reason for hiding this comment

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

these should all just be symlinks to ../../utils/Makefile

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for fixing. Some of the other feature tests still have their own Makefile such as
decode_shared_matrix_conn_gen_proceduralg_dense_egp/Makefile
decode_shared_matrix_conn_gen_proceduralg_dense/Makefile
var_init_egp/Makefile
decode_matrix_globalg_bitmask_optimised/Makefile
... and a few more. Is that on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for fixing!
Some other feature tests still have their own Makefile such as
decode_matrix_globalg_bitmask_optimised/Makefile
... is that on purpose?

tnowotny and others added 3 commits November 17, 2021 19:13
Added some basic documentation.
I have briefly tested that the pygenn wrapper works for $(addToPre) and set_pre_target_var.
This should now complete the $(addToPre) pull request.
@neworderofjamie neworderofjamie merged commit ed144cd into master Nov 18, 2021
@neworderofjamie neworderofjamie deleted the addToPre branch November 18, 2021 09:22
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