Reenable the connection matrix storage for the DBS structures

[MasonProtter]

@gabrevaya I checked the firing rates and whatnot with this change and it looks good to me, but without tests it's hard to know. Can you take a look at this and tell me if it's good?

---

CI may complain about this PR until #454 is merged, because there are some previously broken GraphDynamics tests that are now fixed upstream and #454 reenables them

[gabrevaya]

@gabrevaya I checked the firing rates and whatnot with this change and it looks good to me, but without tests it's hard to know. Can you take a look at this and tell me if it's good?

When you asked me for mean firing rate test I thought you meant tests for the mean_firing_rate function but I realize you are referring to tests for the basal ganglia model checking their mean firing rates, right?

If we want it to be robust to some small changes (like floating point errors due to different calculations or something that makes the randomness to change) we would need to for the simulations to run for a long timespan, which would add more computation time and resources consumption for the tests. Maybe I can make the tests with just a few neurons in each population.

Would you prefer a separate firing rate test for each composite blox, a single one for all the populations connected, or only a test for FSI (which would test both adam_connection_matrix and adam_connection_matrix_gap)?

[MasonProtter]

When you asked me for mean firing rate test I thought you meant tests for the mean_firing_rate function but I realize you are referring to tests for the basal ganglia model checking their mean firing rates, right?

Ah yeah, sorry for the confusion.

If we want it to be robust to some small changes (like floating point errors due to different calculations or something that makes the randomness to change) we would need to for the simulations to run for a long timespan, which would add more computation time and resources consumption for the tests. Maybe I can make the tests with just a few neurons in each population.

Would you prefer a separate firing rate test for each composite blox, a single one for all the populations connected, or only a test for FSI (which would test both adam_connection_matrix and adam_connection_matrix_gap)?

Yeah, true. I think doing a single test per population should be fine, since the problematic change here was internal to the population and didn't require interactions.

[gabrevaya]

I added the firing rate tests, checking if an average over an ensemble of 5 simulations is approximate to their corresponding mean firing rates, with tolerances of 2.5*std. I calculated those mean firing rates and stds on the original implementation, over 100 whole repetitions (creating the populations from scratch in order to account for different random connections between the neurons). I'm testing isolated populations, as you suggested, including 10 neurons each, integrating for 6500 ms and trimming the initial 1000 ms to skip the transient.

The tests pass for all populations except for FSI. So I guess there might be a problem with the adam_connection_matrix_gap because FSI is the only one that uses gap junction connections. Actually, this only means that we get different results when using the two approaches. Maybe your way is correct and the original version had some issue.

[MasonProtter]

Okay so the FSI stuff is now fixed!

[harisorgn]

Can we get a dispatch working for blox::Union{CompositeBlox, AbstractVector{<:AbstractNeuronBlox}} probably using get_components and calling this dispatch internally?

[harisorgn]

Also a dispatch working with a simple solution instead of ensemble, although this could be sorted here too.

[harisorgn]

Finally I would rename this just firing_rate both for brevity and because you are not just returning the mean. The function should work with a single AbstractNeuronBlox too where it returns a single rate (there are no more bloxs to average over).

[harisorgn]

The function should work with a single AbstractNeuronBlox too where it returns a single rate (there are no more bloxs to average over).

Ok this is not always true, my bad. You can average over the solutions in an ensemble.

[harisorgn]

I am not a big fan of tuple outputs like these. Particularly when we add a dispatch for a single solution, the std_fr can become meaningless, except for the case of a CompositeBlox or a vector or bloxs where the average would be over neurons. Given that this function is relatively fast, would it be too bad to have a separate std function that has dispatches only for combinations of inputs that are meaningfull, e.g.

• (blox::Neuron_or_NeuralMass, ::EnsembleSolution)
• (blox::Union{Composite, Vector{<:Neuron_or_NeuralMass}, ::SingleSolution)
• and maybe (blox::Union{Composite, Vector{<:Neuron_or_NeuralMass}, ::EnsembleSolution) where we'll need to specify over what exactly we average.

[harisorgn]

My comments are not relevant for the original purpose of this PR, so you could merge without addressing them here. I just added them here because a new mean_firing_rate dispatch was committed.

Thanks for adding firing rate tests. When we finish adding all dispatches of that function we should test extracting firing rates from components and manually average them to compare against extracting firing rates from composite bloxs (that contain the components).

[gabrevaya]

@MasonProtter Thanks for fixing the FSI stuff!

@harisorgn Thanks for all your comments, I'll make the suggested changes :)

I agree, we can already merge this PR and continue working on the firing rates tomorrow on a different PR.

[MasonProtter]

Great, thanks guys!