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 hashing bugs #508

Merged
merged 4 commits into from
Apr 27, 2022
Merged

Fix hashing bugs #508

merged 4 commits into from
Apr 27, 2022

Conversation

neworderofjamie
Copy link
Contributor

@neworderofjamie neworderofjamie commented Mar 28, 2022

When calculating the hashes used to merge groups together for initialization, usually all that we care about is the names and types of the state variables and how they are initialised but with SIMT backends where additional data structures are required for synapse dynamics/postsynaptic updates this is not enough. This fix simply includes whether weight update models include synapse dynamics and postsynaptic update code in the hash used to determine mergeability - this ignores the backend specific-details so will sometimes split initialization unnecessarily on CPU but fixes this (nasty) issue.

Furthermore, while trying to write a unit test to test this, I found a subtler issue that could also potentially cause issues. Essentially hashing an empty container (std::vector, std::string etc) had no effect on the hash so the following code:

Utils::updateHash("", hash);
Utils::updateHash("test", hash);

generates the same hash as:

Utils::updateHash("test", hash);

I've fixed this by hashing the size of the container before hashing the contents (which, one way or another, involves looping through the bytes of the contents). This PR fixes both issues.

@neworderofjamie neworderofjamie added this to the GeNN 4.7.1 milestone Mar 28, 2022
@neworderofjamie neworderofjamie changed the title include whether synapse dynamics or postlearn are empty in wu init hash Fix hashing bugs Apr 4, 2022
@neworderofjamie neworderofjamie requested a review from tnowotny April 4, 2022 10:38
@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #508 (fa1c9f2) into master (09b74db) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
- Coverage   87.03%   87.03%   -0.01%     
==========================================
  Files          84       84              
  Lines       18108    18115       +7     
==========================================
+ Hits        15761    15767       +6     
- Misses       2347     2348       +1     
Impacted Files Coverage Δ
include/genn/genn/gennUtils.h 100.00% <100.00%> (ø)
src/genn/genn/synapseGroup.cc 85.51% <100.00%> (+0.05%) ⬆️
src/genn/backends/cuda/optimiser.cc 72.60% <0.00%> (-0.18%) ⬇️

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 09b74db...fa1c9f2. 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, makes sense. Nasty issues those ...

@neworderofjamie neworderofjamie merged commit 8d780b4 into master Apr 27, 2022
@neworderofjamie neworderofjamie deleted the init_hash_fix branch April 27, 2022 17:05
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