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

Take multiplicity into account in local_spike_counter #1101

Merged
merged 2 commits into from
Jan 23, 2019

Conversation

stinebuu
Copy link
Contributor

With this PR, the local_spike_counter will also add the multiplicity so that we get the correct spike count.

This fixes #1100.

@stinebuu stinebuu added T: Bug Wrong statements in the code or documentation ZC: Kernel DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Jan 10, 2019
Copy link
Contributor

@suku248 suku248 left a comment

Choose a reason for hiding this comment

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

Thanks @stinebuu !
I think the test could be simplified as described below.

Description:
Creates a parrot neuron, which receives one spike with multiplicity two from a spike generator. The parrot neuron should then emit a spike with multiplicity two, which should be counted as two spikes by the local spike counter.

@jougs this PR is similar to #1105 . Could you please take a look?

@suku248 suku248 requested a review from jougs January 23, 2019 09:06
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Other than that it looks really terrible with all these cvgidcollections sprinkled throughout the code, I think this is a good fix and the best test for it until the full glory of the GidCollection is unleashed. Many thanks!

@stinebuu
Copy link
Contributor Author

@suku248 The test in this PR creates 4000 source parrot neurons and 10 target parrot neurons and the source neurons receive spikes from a poisson_generator, not a spike_generator. With the poisson_generator some of the spikes will have multiplicity greater than one. Should I update the test to just have two parrot neurons and use a spike_generator with multiplicity two? It would make the test more explicit. Or would you be ok with keeping the test as is, and having this description:

Description:
Creates 4000 source parrot neurons and 10 target parrot neurons. The source parrots receive spikes from a poisson generator, some of which will have multiplicity greater than one. The source neurons should then emit a spike with multiplicity greater than one to the target neurons, which should be counted as multiple spikes by the local spike counter.

@suku248
Copy link
Contributor

suku248 commented Jan 23, 2019

@stinebuu As this is a regression test, I'd prefer it to be as minimalist as possible. A minimalist test that exposes the bug (spikes of parrot neurons with multiplicity > 1 are not counted multiple times) is the one that I described above: One spike generator is connected to one parrot neuron. The spike generator feeds one spike with multiplicity two to the parrot neuron, which then emits a spike with multiplicity two. In the current master, the local spike counter only counts one spike, which is incorrect 🐛. With this PR, the local spike counter should count two spikes 🦋.

@stinebuu
Copy link
Contributor Author

@suku248 the emojis are making me so happy! I have now simplified the test, which had the added bonus of loosing the scattered cvgidcollections, @jougs 😁

@suku248 suku248 merged commit fd576ef into nest:master Jan 23, 2019
@stinebuu stinebuu deleted the local_spike_counter_multiplicity branch February 1, 2019 10:57
@terhorstd terhorstd changed the title local_spike_counter takes multiplicity into account Take multiplicity into account in local_spike_counter Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Bug Wrong statements in the code or documentation ZC: Kernel DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

local_spike_counter does not handle multiplicity
3 participants