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

[MRG] Fix the meaning of `i` and `j` in synapses connecting to/from other synapses #854

Merged
merged 2 commits into from Jun 16, 2017

Conversation

Projects
None yet
2 participants
@mstimberg
Member

mstimberg commented Jun 14, 2017

This is a rather marginal use case, but in our astrocyte modeling, we need Synapses that connect to or from other Synapses objects. This works fine in principle, but currently you cannot use i and j in such Synapses, e.g. in connect calls. In most cases, you should not use them directly anyway (because the "index" of a synapse is not very meaningful), but currently it might raise an error or do something completely wrong. The reason is that for a connection from a Synapses object, i will refer to i of the source Synapses which itself refers to i of the synapses's source. This will however be indexed as if it were a variable of the "source synapse". This will do the right thing only by accident, and most of the time will raise an error about incorrect indexing.

This PR fixes this by making i and j not refer to i of the source/target group, but instead just make them an alias for the pre-/postsynaptic indices. We did something like this before but this approach unfortunately does not work when connecting from/to subgroups, so there's a bit of special handling code for that. Also, i and j are now indexed as "synaptic variables", which throws off some analysis code which I had to fix accordingly.

All in all this is not the elegant solution I hoped for, but I think it is still reasonably clean and worth having.

@mstimberg mstimberg changed the title from Fix the meaning of `i` and `j` in synapses connecting to/from other synapses to [MRG] Fix the meaning of `i` and `j` in synapses connecting to/from other synapses Jun 14, 2017

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Jun 15, 2017

Member

Might take a while for me to get my head around this one...

Member

thesamovar commented Jun 15, 2017

Might take a while for me to get my head around this one...

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Jun 15, 2017

Member

Can you explain what the current and proposed behaviour is in these cases?

  • Synapses S1 from A to B. Synapses S2 from C to S1. What do i and j refer to in S2?
  • Synapses S1 from A to B. Synapses S2 from C to D. Synapses S3 from S1 to S2. What do i and j refer to in S3?

Can we not distinguish all the relevant possibilities with i_pre, i_post and i_syn? This would give the following mapping for the cases above:

  • i_pre either doesn't work or refers to C's neuron index. i_syn would refer to C's neuron index. i_post would refer to A's neuron index.
  • i_pre would refer to A, i_syn would refer to S1's synapse index, i_post would refer to C. Similarly j_pre, j_syn, j_post would refer to B, S2, D.
Member

thesamovar commented Jun 15, 2017

Can you explain what the current and proposed behaviour is in these cases?

  • Synapses S1 from A to B. Synapses S2 from C to S1. What do i and j refer to in S2?
  • Synapses S1 from A to B. Synapses S2 from C to D. Synapses S3 from S1 to S2. What do i and j refer to in S3?

Can we not distinguish all the relevant possibilities with i_pre, i_post and i_syn? This would give the following mapping for the cases above:

  • i_pre either doesn't work or refers to C's neuron index. i_syn would refer to C's neuron index. i_post would refer to A's neuron index.
  • i_pre would refer to A, i_syn would refer to S1's synapse index, i_post would refer to C. Similarly j_pre, j_syn, j_post would refer to B, S2, D.
@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Jun 15, 2017

Member

This was more intended as a "quick fix", so I did not think about a new syntax. It is certainly not trivial to wrap around the concept of presynaptic/postsynaptic indexes when the source/target is a synapse itself, but I think the approach taken with this PR is the least confusing you could hope for (again, without introducing new syntax). Here's a table showing how to access various stuff for your second example (i.e. S1 = A <-> B, S2 = C <-> D, S3 = S1 <-> S2). The last line shows the access from S3, which looks sensible to me and covers everything you could possibly need. I'm not sure that replacing S3.i and S3.j by S3.i_syn/S3.j_syn would make things necessarily clearer...:

To access A_i S1_i B_i C_i S2_i D_i
from
S1 S1.i S1.j
S2 S2.i S2.j
S3 S3.i_pre S3.i S3.j_pre S3.i_post S3.j S3.j_post

(Note that you could also access e.g. S1.j as S1.i_post, but let's not make things even more complicated...).

In current master, S3.i_pre, S3.i_post, S3.j_pre, S3.j_post simply don't work (there's no such attribute), and S3.i and S3.j might throw an indexing error depending on the number of synapses in S1 or S2 compared to the number of neurons in A or D.

Member

mstimberg commented Jun 15, 2017

This was more intended as a "quick fix", so I did not think about a new syntax. It is certainly not trivial to wrap around the concept of presynaptic/postsynaptic indexes when the source/target is a synapse itself, but I think the approach taken with this PR is the least confusing you could hope for (again, without introducing new syntax). Here's a table showing how to access various stuff for your second example (i.e. S1 = A <-> B, S2 = C <-> D, S3 = S1 <-> S2). The last line shows the access from S3, which looks sensible to me and covers everything you could possibly need. I'm not sure that replacing S3.i and S3.j by S3.i_syn/S3.j_syn would make things necessarily clearer...:

To access A_i S1_i B_i C_i S2_i D_i
from
S1 S1.i S1.j
S2 S2.i S2.j
S3 S3.i_pre S3.i S3.j_pre S3.i_post S3.j S3.j_post

(Note that you could also access e.g. S1.j as S1.i_post, but let's not make things even more complicated...).

In current master, S3.i_pre, S3.i_post, S3.j_pre, S3.j_post simply don't work (there's no such attribute), and S3.i and S3.j might throw an indexing error depending on the number of synapses in S1 or S2 compared to the number of neurons in A or D.

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Jun 15, 2017

Member

Ah OK, so the way it is working with this PR is what I suggested except for dropping the _syn postscript? In that case, I think that's fine. Will quickly review the code now.

Member

thesamovar commented Jun 15, 2017

Ah OK, so the way it is working with this PR is what I suggested except for dropping the _syn postscript? In that case, I think that's fine. Will quickly review the code now.

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Jun 15, 2017

Member

Ah OK, so the way it is working with this PR is what I suggested except for dropping the _syn postscript?

Yes, it seems to! I have to admit I did not even realize that the i_pre, etc. syntax now works in this way (or that it did not work before) before testing it for that reply. The main point of the PR was really that S3.i and S3.j was sometimes raising an index error (and for the same reason, their use in S3.connect(...) failed). But it is certainly a nice side effect of my fix :) I think I'll add another test for this before merging.

Member

mstimberg commented Jun 15, 2017

Ah OK, so the way it is working with this PR is what I suggested except for dropping the _syn postscript?

Yes, it seems to! I have to admit I did not even realize that the i_pre, etc. syntax now works in this way (or that it did not work before) before testing it for that reply. The main point of the PR was really that S3.i and S3.j was sometimes raising an index error (and for the same reason, their use in S3.connect(...) failed). But it is certainly a nice side effect of my fix :) I think I'll add another test for this before merging.

@mstimberg mstimberg merged commit ce1de01 into master Jun 16, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mstimberg mstimberg deleted the synapse_synapse_connections_i_j_fixes branch Jun 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment