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

Fix batch init #509

Merged
merged 3 commits into from
Apr 27, 2022
Merged

Fix batch init #509

merged 3 commits into from
Apr 27, 2022

Conversation

neworderofjamie
Copy link
Contributor

@neworderofjamie neworderofjamie commented Apr 14, 2022

When implementing batching I wrote a helper function called genVariableFill for initialising variables, taking account of delays and batching. However, for some reason, I forgot to actually use this when initializing inSyn, denDelay (which was subsequently copy-pasted into the initialisation of revInSyn). CUDA seems to zero newly allocated memory so, generally, this has minimal effect but, if you reinitialise your model, you may be left with large residual neural input currents in some batch instances (which, if you're using dendritic delays will continue to appear for max dendritic delay timesteps!).

This finally fixes the issues we were seeing in Felix Schmitt's model - big thanks to him for testing my various speculative fixes and suggesting that (re-)initialization could be the problem!

@neworderofjamie neworderofjamie added this to the GeNN 4.7.1 milestone Apr 14, 2022
@codecov
Copy link

codecov bot commented Apr 14, 2022

Codecov Report

Merging #509 (ba197f2) into master (480e70a) will decrease coverage by 5.60%.
The diff coverage is 85.29%.

@@            Coverage Diff             @@
##           master     #509      +/-   ##
==========================================
- Coverage   87.03%   81.43%   -5.61%     
==========================================
  Files          84       74      -10     
  Lines       18108    14916    -3192     
==========================================
- Hits        15761    12147    -3614     
- Misses       2347     2769     +422     
Impacted Files Coverage Δ
src/genn/genn/code_generator/initGroupMerged.cc 95.18% <85.29%> (+0.04%) ⬆️
src/genn/backends/single_threaded_cpu/backend.cc 0.91% <0.00%> (-55.28%) ⬇️
...nclude/genn/backends/single_threaded_cpu/backend.h 31.57% <0.00%> (-50.78%) ⬇️
include/genn/genn/synapseMatrixType.h 60.00% <0.00%> (-15.00%) ⬇️
...clude/genn/genn/code_generator/supportCodeMerged.h 79.16% <0.00%> (-10.49%) ⬇️
include/genn/genn/code_generator/teeStream.h 76.92% <0.00%> (-8.08%) ⬇️
include/genn/genn/code_generator/substitutions.h 72.97% <0.00%> (-5.76%) ⬇️
...nclude/genn/genn/initToeplitzConnectivitySnippet.h 44.44% <0.00%> (-5.56%) ⬇️
src/genn/genn/code_generator/generateModules.cc 89.61% <0.00%> (-5.46%) ⬇️
src/genn/genn/code_generator/modelSpecMerged.cc 90.09% <0.00%> (-5.44%) ⬇️
... and 63 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 480e70a...ba197f2. 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.

Looks good (I haven't checked what the genVariableFill function does, have to trust you on that to not descend too deeply in limited time)

@neworderofjamie neworderofjamie merged commit f63ab48 into master Apr 27, 2022
@neworderofjamie neworderofjamie deleted the fix_batch_init branch April 27, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants