Custom update merge fix #557
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportBase: 88.80% // Head: 88.81% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #557 +/- ##
=======================================
Coverage 88.80% 88.81%
=======================================
Files 76 76
Lines 12344 12354 +10
=======================================
+ Hits 10962 10972 +10
Misses 1382 1382
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
# Conflicts: # src/genn/genn/models.cc
…chedness of the variable references (overriding ``VarAccessDuplication``)
tnowotny
approved these changes
Nov 23, 2022
|
Yeah - I feel maybe there might be some extra constraints that could be added which would simplify the whole system somewhat e.g. I'm pretty sure I haven't yet had a use-case for a custom update that simultaneously references variables in different populations of the size (although this is supported) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the course of my structural plasticity implementing, I found not one but two bugs in the variable reference/custom update stuff!
Merging
In general, when merging together custom updates, what the variable references target doesn't really matter - they're just pointers. However, whether variable references point to variables with
VarAccessDuplication::DUPLICATE,VarAccessDuplication::SHAREDetc changes the required index calculating code so this needs to be considered. This PR includes the variable duplication mode in the hashes used for merging groups and extends the unit tests to better cover corner cases in this area.References to custom update variables
Unlike synapses and neurons which are always batched if the model is, if custom updates are batched or not depends on whether they are attached to
VarAccessDuplication::DUPLICATEvariables (and whether the model is batched). This means if you make references to these variables from other custom updates whether these 'downstream' custom updates should be batched should depend on whether the upstream custom update was batched as well as the mode of the variables. This PR adds aisBatchedmethod to each variable reference and extends the unit tests to cover this.