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 multiple issues with sparse synapse index narrowing #460

Merged
merged 4 commits into from
Sep 22, 2021

Conversation

neworderofjamie
Copy link
Contributor

In #274 I implemented support for 'narrow' sparse synapse indices so, if the postsynaptic population was < 65536 neurons, uint16_t would be used and if < 256 neurons, uint8_t. However:

  1. PyGeNN always tried to access the sparse synapse indices as uint32_t - it now callsSynapseGroup::getSparseIndType to get correct type
  2. In the generated code for pushXXConnectivityToDevice and pullXXConnectivityToDevice, unsigned int was always being used - this also now uses the correct type.

@neworderofjamie neworderofjamie added this to the GeNN 4.6.0 milestone Sep 20, 2021
@codecov
Copy link

codecov bot commented Sep 20, 2021

Codecov Report

Merging #460 (c80ad12) into master (0cff2ed) will increase coverage by 0.02%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #460      +/-   ##
==========================================
+ Coverage   87.23%   87.25%   +0.02%     
==========================================
  Files          78       78              
  Lines       17057    17056       -1     
==========================================
+ Hits        14879    14883       +4     
+ Misses       2178     2173       -5     
Impacted Files Coverage Δ
src/genn/genn/synapseGroup.cc 82.74% <85.71%> (+0.85%) ⬆️
include/genn/genn/synapseGroup.h 90.90% <100.00%> (ø)
src/genn/genn/code_generator/generateRunner.cc 95.85% <100.00%> (ø)

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 0cff2ed...c80ad12. 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.

Oh dear ... good catch there. I suspect this came out from the guy's report about huge post-indices fro a small post-population?
Can you tell that you have caught everything now (I unfortunately can't ...)?

@neworderofjamie
Copy link
Contributor Author

It did indeed...The updated feature test definitely now covers the second bug but, PyGeNN is currently not well tested as most of the testing (aside from that done at a higher level by PyNNGeNN or mlGeNN) happens at the C++ level. For the low-level unit tests I think that is advantageous but, as part of the test revamp we've been talking about, maybe porting all the feature tests to PyGeNN is the solution.

@neworderofjamie neworderofjamie merged commit 60f01cf into master Sep 22, 2021
@neworderofjamie neworderofjamie deleted the pygenn_narrow_sparse_fix branch September 22, 2021 08:36
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