Connectivity_matrix_connect update

[kyralianaka]

This PR resolves #486 so that views with global indices of different ranges can be used to connect cells with an adjacency matrix (and adds a test).

I also removed the random selection of compartments on the post synaptic cell because I don't quite see the benefit to doing this as opposed to just using the first compartment of the first branch, and the random selection in list comprehension was really slowing down my building of large networks. This being said, I would be interested in having connectivity_matrix_connect take a matrix (num pre comps x num post comps) instead of (num pre cells x num post cells) for more fine control of the connectivity... I can open another issue about this as enhancement.

[jnsbck]

This was also fixed by #488, which I merged already.
The random selection is consistent with the other cell level connection methods. Also the first of the first of the first will not work with single comp cells. I agree that we should make it faster though if it slows down the whole thing.

The comp2comp connectivity matrix connect that you're proposing is basically already possible using connect since it will not just take single comps but also n comps. Something like the following should work:

con_matrix = some_boolean_matrix
pre_inds, post_inds = np.where(con_matrix)
pre_comps = net.select(pre_inds)
post_comps = net.select(post_inds)

connect(pre_comps, post_comps, TestSynapse())

[kyralianaka]

I am happy to go through the other methods and remove the random selection; it will always make connecting networks faster and avoid having to look for the postsynaptic compartments.

I'm not sure why you say it won't work for single compartment cells, I can change the test to single compartment cells and have no problem.

For the comp2comp mat, that's fine, no need to add then.

In general I would just like to make this function easier to read, mainly by not using so many types of indices for the same cells ("from_idx", "pre_cell_inds", and "global_pre_cell_inds"). I think there are even more simplifications I can do, so I will do those and commit again (will also remove the rand selection).

[kyralianaka]

I also think the added test is really important especially if you make more plans to change synapse indexing

[jnsbck]

I agree that it will make networks faster, but I would not make it the default. I'd be fine with making it a toggle though, i.e.
post_comp="random" per default and sth. like post_comp=1 for branch 0, comp 1. If we add this, we should keep in mind that branches can have different numbers of segments tho.

Ah I see, you mean 0th comp rather than 1st. In that case I am strongly against hard coding this, since we would always connect soma to soma.

I'd be surprised if you can make rewrite it without `"from_idx", "pre_cell_inds", and "global_pre_cell_inds", but if you have an idea how if can work its great if you can make the function easier to read.

Could you briefly explain what the additional test is covering that is not covered by the other cases? Also please add some more doc/comments :)

[kyralianaka]

I am strongly for making it the default haha, but open to giving the option for random selection. Your concern about always connecting soma to soma doesn't quite make sense to me because the soma doesn't have to be and often isn't the zeroith compartment (when self-defining cells, I could be wrong about swc loaded cells, in which case I would reconsider).

Re all the indices, I renamed things and I think the most clarifying aspect is distinguishing between global_cell_inds and global_comp_inds. I also really like it if the pre and post indices are handled the same way because this makes the function more modular.

I just added some comments to the tests, they are in the test_connectivity_matrix_connect() function, but you can let me know if anything is still unclear. I might still go add even more comments :)

[kyralianaka]

Thinking about the way cells are typically constructed, would it actually maybe make sense to default connect the last compartment of the presynaptic cell to the zeroith compartment of the post-synaptic cell? (yes before I meant zeroith when I said first sorry for the confusion)

[kyralianaka]

Another thing, the test_sparse_connect() function uses this:
assert all( [ 63, 59, 65, 86, 80, 58, 92, 85, 168, 145, 189, 153, 180, 190, 184, 163, 159, 179, 182, ] )
so it will always pass... I will fix that here

[kyralianaka]

I updated all of the connectivity functions now so that they all index in the same way. It probably looks more repetitive now, but I would argue that that's a good thing as a more modular design to avoid potential future bugs.

I haven't yet included the option to randomly select the postsynaptic compartment, but I guess we should talk about that as a group and get everyone's opinions.

[kyralianaka]

Two tests are currently failing because changing the post_comp_index of the fully connected network changes the voltage solutions of simulations. I will wait to fix this then until we talk about the post_comp_index scheme.

[michaeldeistler]

+1 for giving the option of random connection vs first-to-first. We should do it consistently for all connection methods though. I am fine with having first-to-first as the default. I am against connecting first to last.

As for the connectivity_matrix_connect to be a (comp, comp) matrix: I think this would be great, but doing it naively will blow up memory. Instead, we should support matrices to be passed in csr or csc formats.

[kyralianaka]

I just added back the option to randomly select the post-synaptic compartment; setting this to true makes most of the old tests pass (except where I changed other things).

The other thing I changed was the way of sparsely connecting the network because the previous method randomly sampled pre and post cells separately, which resulted in a lot of duplicate connections. Now the connectivity matrix is generated by independent Bernoulli trials, consistent with common random graph generation methods. The one disadvantage of this method is that it could be memory intensive for extremely large numbers of pre and post synaptic cells, but since the matrix is local, I'm hopeful that it won't cause problems.

I might look into the (comp, comp) matrix situation in another pull request since it will soon become relevant to my project, but hopefully we can update these functions first :)

[michaeldeistler]

Cool! When you are done please request a review!

[michaeldeistler]

Awesome, thanks! Could you report the runtime of random_post_comp = True in the PR description and compare it to the runtime before this PR? Especially for large networks.

(And ideally also add the timing for random_post_comp=False (after this PR).

[michaeldeistler]

not clear what you mean here

[kyralianaka]

Here I just mean that I ran network.vis() in a notebook many times to make sure that the connectivity looks as expected, but I can remove these comments

[michaeldeistler]

again, unclear

[michaeldeistler]

what is a view range?

[kyralianaka]

This addresses bug #486 where connectivity_matrix_connect() would not work with e.g.

pre = net.cell(list(range(2, 5)))
post = net.cell(list(range(5, 7)))

but did when the pre range started at 0, e.g.

pre = net.cell(list(range(0, 4)))
post = net.cell(list(range(4, 8)))

[kyralianaka]

I changed the wording a bit

[michaeldeistler]

is there a test for random_post_comp=False? At least for fully_connect, but also for the others? At least to check that they do not break?

[kyralianaka]

All of the tests in test_connection.py are with random_post_comp=False, but the tests that used connection.py everywhere else (test_grad.py and test_basic_modules.py) use random_post_comp=True with fully connect (so that the simulation results are the same as before). I could add tests for random_post_comp=True to test_connection.py -- would this then be enough coverage?

[michaeldeistler]

@kyralianaka please request a review when ready.

[jnsbck]

Had a quick look and left some comments, not sure when you requested the review, if it was recent or months ago. Also make sure to rebase once ready, since there have been a lot of merged prs lately

[jnsbck]

make sure to reset this after, in case the user is in global scope

[jnsbck]

When I implemented the connect API, @michaeldeistler and me had discussed not wanting to do this, since this can be really bad for large, sparse networks. Did we change our mind about doing it this way?

[jnsbck]

I agree its cleaner and easier to parse, but not efficient.

[michaeldeistler]

Good catch Jonas! Let's avoid building a n x n matrix.

[kyralianaka]

I was looking into this for quite a while, and all of the ways that are memory efficient are extremely time intensive, and vice versa, so I came up with a way to split the cost a bit... generating blocks of random matrix at a time. I am still thinking of how it could be prettier, but the resource management seems to be good

[michaeldeistler]

What is the reason we cannot do it as it was before? If really would like to avoid memory scaling with O(N^2).

[kyralianaka]

The function before created many duplicate connections between the same cells, and simply filtering these out would remove a lot of connections (inefficient) and make the p value entered somewhat meaningless (more problematic). Also p=1.0 did not create a fully connected graph, far from it I might add. Resource efficient methods for constructing sparse random graphs are highly sought after and typically trade off computation speed and memory, so I tried to write a function that balances the two. The function now isn't O(N^2) per se, the largest matrix it creates is 100x100. I can keep looking for better algorithms in the future, but for now the function at least fixes the bug of duplicate connections and restores the traditional meaning of p.

[kyralianaka]

I'm also thinking now that just removing sparse_connect() might not be so bad... I think we might have used connectivity matrix connect or something else in the RNN experiments anyway

[jnsbck]

should we rename fully_connect to fully_connect_cells? Might be less confusing, since otherwise connections are made comp 2 comp

[kyralianaka]

Thanks for the feedback, I've done a bit of runtime testing and there is still some room for improvement, but my goal is still to make it as readable as possible (so that it's easy to write more functions like this) while not too slow. I'll work on this on Monday and think a bit more about restructuring

[kyralianaka]

An update on the timing:

	PR	original
fc 0-0	2.485 s
fc rand	2.205 s	2.00 s
sparse 0-0	3.16 s
sparse rand	3.24 s	10963.41 s (3 h)

The random selection does actually seem to be faster now, so I'll just make that the default again unless that changes. Also shifting back towards the old fully connect which didn't have any problems other than not being so easy to parse.