Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

This is a fix for issue #136. #138

Merged
merged 3 commits into from
Jan 11, 2022
Merged

Conversation

tnowotny
Copy link
Contributor

@tnowotny tnowotny commented Jan 5, 2022

The root cause for the problem was that for situations where sub-populations of neuron populations were connected
and a state monitor for the synapses instantiated, the pre-template prep code would identify the size of the connected sub-populations for the srcN and trgN variables of the stateMonitorModel. These variables are then used for the argument list of the convert_dense_matrix_2_dynamic_arrays and convert_sparse_synapses_2_dynamic_arrays calls that translate GeNN synaptic data into brian format synaptic data. Because brian2genn implements sub-populations only by connectivity matrices between full populations, the conversion functions need the size of the full populations, not of the connected sub-populations.
I believe the srcN and trgN variables are solely for the purpose of these conversions, so should not be needed to represent sub-population size elsewhere.

… for situations where sub-populations of neuron populations were connected

and a state monitor for the synapses instantiated, the pre-template prep code would identify the size of the connected sub-populations for the
srcN and trgN variables of the stateMonitorModel. These variables are then used for the argument list of the convert_dense_matrix_2_dynamic_arrays and
convert_sparse_synapses_2_dynamic_arrays calls that translate GeNN synaptic data into brian format synaptic data. Because brian2genn implements sub-populations
only by connectivity matrices between full populations, the conversion functions need the size of the full populations, not of the connected sub-populations.
I believe the srcN and trgN variables are solely for teh purpose of these conversions, so should not be needed to represent subpopulation size elsewhere.
@tnowotny tnowotny requested a review from mstimberg January 5, 2022 15:48
@tnowotny
Copy link
Contributor Author

tnowotny commented Jan 5, 2022

Oh dear ... I see the tests are failing but I am afraid it is likely due to a change in Brian 2. The tests seem to pass locally with brian 2.4.2
And I have had problems with brian2genn and brian 2.5* ...

@kernfel
Copy link
Contributor

kernfel commented Jan 6, 2022

Oops, based on the b2g release notes, I assumed compatibility with later brian2 versions was not guaranteed anyway, and didn't report my own failures on later/current brian2. Which is to say: Can confirm, brian2 > 2.4.2 breaks brian2genn.

@mstimberg
Copy link
Member

Apologies, Brian2GeNN is indeed not compatible yet with Brian 2.5, but requirements and test suite do not reflect this. I'll try to take care of this tomorrow.

Copy link
Member

@mstimberg mstimberg 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 to me 👍

@mstimberg
Copy link
Member

I pushed changes to master to make Brian2GeNN compatible with Brian 2.5 and merged the changes into this branch. All tests pass, so this is good to merge.

@mstimberg mstimberg merged commit 16f073d into master Jan 11, 2022
@mstimberg mstimberg deleted the fix_synapses_subgroup_monitors branch January 11, 2022 09:59
@kernfel
Copy link
Contributor

kernfel commented Jan 11, 2022

Excellent, thank you both!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants