Implemented flag for symmetric (one-to-one) connections #315

Merged
merged 18 commits into from May 4, 2016

Conversation

Projects
None yet
4 participants
@janhahne
Contributor

janhahne commented Apr 20, 2016

As discussed in the connect interface status meeting (24.03) I implemented a symmetric flag for the 'conn_spec' in order to be able to create symmetric connections (e.g. required for gap-junctions).

So far the implementation only works for the One-to-One connection rule and throws an 'NotImplemented' when called with the other connection rules.

Still missing: unittests and documentation

nestkernel/conn_builder.cpp
@@ -369,13 +371,29 @@ nest::ConnBuilder::change_connected_synaptic_elements( index sgid,
void
nest::ConnBuilder::connect()
{
+ if ( symmetric_ && not supports_symmetric() )
+ throw NotImplemented(
+ "So far Connect doesn't support symmetric connections for this "

This comment has been minimized.

@heplesser

heplesser Apr 21, 2016

Contributor

I would remove the "So far". It implies a promise of coming attractions that we may not want to or be able to fulfill.

@heplesser

heplesser Apr 21, 2016

Contributor

I would remove the "So far". It implies a promise of coming attractions that we may not want to or be able to fulfill.

This comment has been minimized.

@jougs

jougs Apr 21, 2016

Contributor

I would even rephrase as "This connection rule does not support symmetric connections" because the function might be called from other sources than just plain Connect. It might also help to include the name of the rule, if that's accessible here. @hannahbos, can you help here?

@jougs

jougs Apr 21, 2016

Contributor

I would even rephrase as "This connection rule does not support symmetric connections" because the function might be called from other sources than just plain Connect. It might also help to include the name of the rule, if that's accessible here. @hannahbos, can you help here?

nestkernel/conn_builder.cpp
+ std::swap( sources_, targets_ );
+ connect_();
+ std::swap( sources_,
+ targets_ ); // not really necessary, but leaves things in clean state

This comment has been minimized.

@heplesser

heplesser Apr 21, 2016

Contributor

Move the comment to a separate line, so that the code fits on one line for clarity. The "not really necessary" is a bit misleading. This is only the case if you call connect() only a single time on a ConnBuilder object. In case of multiple calls (which are not expressly forbidden anywhere), swapping back is essential.

@heplesser

heplesser Apr 21, 2016

Contributor

Move the comment to a separate line, so that the code fits on one line for clarity. The "not really necessary" is a bit misleading. This is only the case if you call connect() only a single time on a ConnBuilder object. In case of multiple calls (which are not expressly forbidden anywhere), swapping back is essential.

This comment has been minimized.

@jougs

jougs Apr 21, 2016

Contributor

I second @heplesser and would remove the comment altogether or change it to "re-establish original state".

@jougs

jougs Apr 21, 2016

Contributor

I second @heplesser and would remove the comment altogether or change it to "re-establish original state".

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 21, 2016

Contributor

@janhahne Nice work! I think the best place for documentation on the autapse, multapse and symmetric flags, which apply to all rules, would be as part of the general Connect documentation in lib/sli/nest-init.sli.

Concerning the tests, I suppose that you have tests already, either in SLI or in Python. If you have them in Python, could you integrate them with the PyNEST tests? @hannahbos is our expert on connectivity tests in Python. A crucial test would be an MPI test, which needs to be written in SLI and is particularly tricky. If you can send me a testing script in either SLI or Py, I can turn it into an MPI test.

Contributor

heplesser commented Apr 21, 2016

@janhahne Nice work! I think the best place for documentation on the autapse, multapse and symmetric flags, which apply to all rules, would be as part of the general Connect documentation in lib/sli/nest-init.sli.

Concerning the tests, I suppose that you have tests already, either in SLI or in Python. If you have them in Python, could you integrate them with the PyNEST tests? @hannahbos is our expert on connectivity tests in Python. A crucial test would be an MPI test, which needs to be written in SLI and is particularly tricky. If you can send me a testing script in either SLI or Py, I can turn it into an MPI test.

nestkernel/conn_builder.cpp
+ if ( symmetric_ )
+ throw NotImplemented(
+ "So far Connect doesn't support symmetric connections in combination "
+ "with structural plasticity." );

This comment has been minimized.

@jougs

jougs Apr 21, 2016

Contributor

Same issues as above

@jougs

jougs Apr 21, 2016

Contributor

Same issues as above

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Apr 21, 2016

Contributor

I also like the code and am happy with merging once we have a test and you addressed the minor comments on the code.

Contributor

jougs commented Apr 21, 2016

I also like the code and am happy with merging once we have a test and you addressed the minor comments on the code.

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Apr 23, 2016

Contributor

@heplesser @jougs I addressed your comments, added a short documentation and a unittest.

The testing that the symmetric connections work properly with one_to_one so far only checks that any connections are created with the target nodes as sources. This might be improved by someone with better knowledge of GetConnections and sli arrays.

There is still one small problem left:

/iaf_neuron 2 Create
1 2 << /rule /all_to_all /symmetric true >> /static_synapse Connect

This sli code does not throw the NotImplemented error, but just ignores the symmetric flag and creates the connection. The same happens for one_to_one (just one direction is created). As soon as one uses gidcollections instead of the most basic version of Connect everything works as expected. In Python all versions of nest.Connect seem to work fine.

In addition I am wondering if we should allow the symmetric flag for all_to_all connections. So far this is disabled, because it is not necessary for calls like nest.Connect( N_all, N_all ) as both directions are created anyway when sources and targets are the same. But in other cases like nest.Connect( group1, group2 ) this might be benefical. What do you think?

Contributor

janhahne commented Apr 23, 2016

@heplesser @jougs I addressed your comments, added a short documentation and a unittest.

The testing that the symmetric connections work properly with one_to_one so far only checks that any connections are created with the target nodes as sources. This might be improved by someone with better knowledge of GetConnections and sli arrays.

There is still one small problem left:

/iaf_neuron 2 Create
1 2 << /rule /all_to_all /symmetric true >> /static_synapse Connect

This sli code does not throw the NotImplemented error, but just ignores the symmetric flag and creates the connection. The same happens for one_to_one (just one direction is created). As soon as one uses gidcollections instead of the most basic version of Connect everything works as expected. In Python all versions of nest.Connect seem to work fine.

In addition I am wondering if we should allow the symmetric flag for all_to_all connections. So far this is disabled, because it is not necessary for calls like nest.Connect( N_all, N_all ) as both directions are created anyway when sources and targets are the same. But in other cases like nest.Connect( group1, group2 ) this might be benefical. What do you think?

lib/sli/nest-init.sli
+ multapses bool - allow self-connections (default: true)
+ autapses bool - allow multiple connections between pairs
+ of neurons (default: true)
+ symmetric bool - also create connection in opposite direction to

This comment has been minimized.

@heplesser

heplesser Apr 23, 2016

Contributor

@janhahne You got autapses and multapses the wrong way around. To avoid confusion, I would change the headline to The following options may be given for all connection rules; not all rules support all options and many rules add rule-specific options.

@heplesser

heplesser Apr 23, 2016

Contributor

@janhahne You got autapses and multapses the wrong way around. To avoid confusion, I would change the headline to The following options may be given for all connection rules; not all rules support all options and many rules add rule-specific options.

+/set1 1 5 cvgidcollection def
+/set2 6 10 cvgidcollection def
+
+% Check that symmetric flag cannot be used with other connection rules then one_to_one

This comment has been minimized.

@heplesser

heplesser Apr 23, 2016

Contributor

"than", not "then"

@heplesser

heplesser Apr 23, 2016

Contributor

"than", not "then"

+% Check that symmetric flag cannot be used with other connection rules then one_to_one
+{
+set1 set2 << /rule /all_to_all /symmetric true >> /static_synapse Connect
+} fail_or_die

This comment has been minimized.

@heplesser

heplesser Apr 23, 2016

Contributor

Typical SLI formatting would indent the code in the brackets by 2 and place fail_or_die on a separate line, separate the tests by empty lines. I will think about how to stuff this into a loop.

@heplesser

heplesser Apr 23, 2016

Contributor

Typical SLI formatting would indent the code in the brackets by 2 and place fail_or_die on a separate line, separate the tests by empty lines. I will think about how to stuff this into a loop.

+} pass_or_die
+
+<< /source [6 7 8 9 10] >> GetConnections
+{empty false eq}assert_or_die

This comment has been minimized.

@heplesser

heplesser Apr 23, 2016

Contributor

Here, you only test that at least one connection is created with 6-10 as source. We need to test that the backward connections are really mirror images of the forward connections. Something like this, not entirely certain (@jougs: Does GetConnections accept GIDCollections as input?):

<< /source set1 >> GetConnections GetStatus /fwd Set
<< /source set2 >> GetConnections GetStatus /bck Set
[ fwd bck ] 
{
  /b def /f def
  f /source get b /target get eq
  b /source get f /target get eq and
  f /delay get b /delay get eq and
  f /weight get b /delay get eq and
} MapThread
true exch { and } Fold
} assert_or_die

The code above depends on the ordering of connections. To be even more general, we would have to go through the forward connections and search for pertaining backward connections and then compare. But I think for now this should be ok.

@heplesser

heplesser Apr 23, 2016

Contributor

Here, you only test that at least one connection is created with 6-10 as source. We need to test that the backward connections are really mirror images of the forward connections. Something like this, not entirely certain (@jougs: Does GetConnections accept GIDCollections as input?):

<< /source set1 >> GetConnections GetStatus /fwd Set
<< /source set2 >> GetConnections GetStatus /bck Set
[ fwd bck ] 
{
  /b def /f def
  f /source get b /target get eq
  b /source get f /target get eq and
  f /delay get b /delay get eq and
  f /weight get b /delay get eq and
} MapThread
true exch { and } Fold
} assert_or_die

The code above depends on the ordering of connections. To be even more general, we would have to go through the forward connections and search for pertaining backward connections and then compare. But I think for now this should be ok.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 23, 2016

Contributor

@janhahne I just realized one more thing we need to handle properly: Parameters. We can do so as follows: ConnParameter gets a new virtual method reset() that throws NotImplemented by default. Scalar parameter classes override it with a function doing nothing, array parameter classes with one resetting next_ to values_->begin(), while random parameters do not override: there is no way to generate the same value sequence twice, so they raise the NotImplemented exception via the base-class method.

To catch problems early, we call reset() on all parameters in the ConnBuilder constructor if symmetric connections are requested.

Contributor

heplesser commented Apr 23, 2016

@janhahne I just realized one more thing we need to handle properly: Parameters. We can do so as follows: ConnParameter gets a new virtual method reset() that throws NotImplemented by default. Scalar parameter classes override it with a function doing nothing, array parameter classes with one resetting next_ to values_->begin(), while random parameters do not override: there is no way to generate the same value sequence twice, so they raise the NotImplemented exception via the base-class method.

To catch problems early, we call reset() on all parameters in the ConnBuilder constructor if symmetric connections are requested.

@hannahbos

This comment has been minimized.

Show comment
Hide comment
@hannahbos

hannahbos Apr 24, 2016

@janhahne thanks for implementing this. You could add a test to test_connect_one_to_one.py in the python test suite. For example something like

def testSymmetricFlag(self):
conn_dict_symmetric = self.conn_dict.copy()
conn_dict_symmetric['symmetric'] = True
self.setUpNetwork(conn_dict_symmetric)
M = hf.get_connectivity_matrix(self.pop2, self.pop1)
# test that connections were created in both directions
hf.mpi_assert(M, np.transpose(M), self)
# test that no other connections were created
hf.mpi_assert(M[1:,1:], np.zeros_like(M[1:,1:]), self)

Regarding the documentation, it would be helpful to add the new flag to the docstring of Connect() in hl_api_connections.py.

@janhahne thanks for implementing this. You could add a test to test_connect_one_to_one.py in the python test suite. For example something like

def testSymmetricFlag(self):
conn_dict_symmetric = self.conn_dict.copy()
conn_dict_symmetric['symmetric'] = True
self.setUpNetwork(conn_dict_symmetric)
M = hf.get_connectivity_matrix(self.pop2, self.pop1)
# test that connections were created in both directions
hf.mpi_assert(M, np.transpose(M), self)
# test that no other connections were created
hf.mpi_assert(M[1:,1:], np.zeros_like(M[1:,1:]), self)

Regarding the documentation, it would be helpful to add the new flag to the docstring of Connect() in hl_api_connections.py.

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Apr 28, 2016

Contributor

@heplesser The latest commit implements your idea for the handling of parameters. Please have a look.

@hannahbos Thank you for the pynest test!

Contributor

janhahne commented Apr 28, 2016

@heplesser The latest commit implements your idea for the handling of parameters. Please have a look.

@hannahbos Thank you for the pynest test!

nestkernel/conn_parameter.h
+ reset() const
+ {
+ throw NotImplemented(
+ "Symmetric connections do not work with random parameters." );

This comment has been minimized.

@heplesser

heplesser Apr 28, 2016

Contributor

All kinds of future parameter types might not support resetting. "Symmetric connections require parameters that can be reset." would be more general.

@heplesser

heplesser Apr 28, 2016

Contributor

All kinds of future parameter types might not support resetting. "Symmetric connections require parameters that can be reset." would be more general.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 28, 2016

Contributor

@janhahne Looks good! Could you add a few more tests, especially

  • a test that provide weight, delay and some other parameter (for stdp_synapse, e.g.) using arrays, to make sure the resetting works
  • a test with random parameter to trigger the exception
Contributor

heplesser commented Apr 28, 2016

@janhahne Looks good! Could you add a few more tests, especially

  • a test that provide weight, delay and some other parameter (for stdp_synapse, e.g.) using arrays, to make sure the resetting works
  • a test with random parameter to trigger the exception
@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Apr 29, 2016

Contributor

@heplesser I improved the exception-message and added tests to trigger the exception.

For the other test I tried the following:

% Check that symmetric flag works properly for one_to_one connections
% containing array-parameters

ResetKernel

/iaf_neuron 10 Create

/set1 1 5 cvgidcollection def
/set2 6 10 cvgidcollection def

{
  set1 set2 
  << /rule /one_to_one 
     /symmetric true >> 
  << /model /stdp_synapse 
     /weight [1.0 2.0 3.0 4.0 5.0]
     /delay [1.5 2.5 3.5 4.5 5.5]
     /alpha [1.2 2.2 3.2 4.2 5.2] >> 
  Connect
}
pass_or_die

<< /source [1 5] Range >> GetConnections /fwd Set
<< /source [6 10] Range >> GetConnections /bck Set

{
  [ fwd bck ] 
  {
    /b Set /f Set
    f /source get b /target get eq
    b /source get f /target get eq and
    f /delay get b /delay get eq and
    f /weight get b /delay get eq and
    f /alpha get b /alpha get eq and
  } MapThread
  true exch { and } Fold
} assert_or_die

but I get the error Connect_g_g_D_D [Error]: BadProperty Cannot handle parameter type. Received arraytype. The documentation of connect says indeed that Parameters for synapses may be fixed single values and random deviate specifications. Is there indeed no way to do this in SLI?

Contributor

janhahne commented Apr 29, 2016

@heplesser I improved the exception-message and added tests to trigger the exception.

For the other test I tried the following:

% Check that symmetric flag works properly for one_to_one connections
% containing array-parameters

ResetKernel

/iaf_neuron 10 Create

/set1 1 5 cvgidcollection def
/set2 6 10 cvgidcollection def

{
  set1 set2 
  << /rule /one_to_one 
     /symmetric true >> 
  << /model /stdp_synapse 
     /weight [1.0 2.0 3.0 4.0 5.0]
     /delay [1.5 2.5 3.5 4.5 5.5]
     /alpha [1.2 2.2 3.2 4.2 5.2] >> 
  Connect
}
pass_or_die

<< /source [1 5] Range >> GetConnections /fwd Set
<< /source [6 10] Range >> GetConnections /bck Set

{
  [ fwd bck ] 
  {
    /b Set /f Set
    f /source get b /target get eq
    b /source get f /target get eq and
    f /delay get b /delay get eq and
    f /weight get b /delay get eq and
    f /alpha get b /alpha get eq and
  } MapThread
  true exch { and } Fold
} assert_or_die

but I get the error Connect_g_g_D_D [Error]: BadProperty Cannot handle parameter type. Received arraytype. The documentation of connect says indeed that Parameters for synapses may be fixed single values and random deviate specifications. Is there indeed no way to do this in SLI?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 3, 2016

Contributor

@janhahne You get the bad property error because you input an array, while NEST expects a doublevector. To create those, use <. ... .>, i.e.,

     /weight <. 1.0 2.0 3.0 4.0 5.0 .>
     /delay <. 1.5 2.5 3.5 4.5 5.5 .>
     /alpha <. 1.2 2.2 3.2 4.2 5.2 .> 
Contributor

heplesser commented May 3, 2016

@janhahne You get the bad property error because you input an array, while NEST expects a doublevector. To create those, use <. ... .>, i.e.,

     /weight <. 1.0 2.0 3.0 4.0 5.0 .>
     /delay <. 1.5 2.5 3.5 4.5 5.5 .>
     /alpha <. 1.2 2.2 3.2 4.2 5.2 .> 
@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne May 3, 2016

Contributor

@heplesser Thank you for the explanation! I added the corresponding test. The only thing still missing is a MPI-test.

Contributor

janhahne commented May 3, 2016

@heplesser Thank you for the explanation! I added the corresponding test. The only thing still missing is a MPI-test.

+/iaf_neuron 10 Create
+
+/set1 1 5 cvgidcollection def
+/set2 6 10 cvgidcollection def

This comment has been minimized.

@heplesser

heplesser May 3, 2016

Contributor

@janhahne I noticed that here an in several other places below you have code outside any [pass|assert]_or_die blocks. This is not really optimal, because it creates dependencies between parts of the code. A better way is to create a little helper function (e.g., /setup, /prepare, or so) that sets up the necessary pieces and the call that from each of the test blocks.

@heplesser

heplesser May 3, 2016

Contributor

@janhahne I noticed that here an in several other places below you have code outside any [pass|assert]_or_die blocks. This is not really optimal, because it creates dependencies between parts of the code. A better way is to create a little helper function (e.g., /setup, /prepare, or so) that sets up the necessary pieces and the call that from each of the test blocks.

+
+ResetKernel
+
+/iaf_neuron 10 Create

This comment has been minimized.

@heplesser

heplesser May 3, 2016

Contributor

Use /iaf_psc_alpha. iaf_neuron may disappear in the future.

@heplesser

heplesser May 3, 2016

Contributor

Use /iaf_psc_alpha. iaf_neuron may disappear in the future.

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne May 4, 2016

Contributor

@heplesser Thanks a lot for the suggestions and the mpi-test! I merged your PR and improved the serial unittest.

Contributor

janhahne commented May 4, 2016

@heplesser Thanks a lot for the suggestions and the mpi-test! I merged your PR and improved the serial unittest.

{
+ << /source [1 5] Range >> GetConnections /fwd Set

This comment has been minimized.

@heplesser

heplesser May 4, 2016

Contributor

This test depends on the previous test to create the network. It would be better to join the previous test with this one. assert is a tighter requirement than pass, so it is not strictly necessary to test first that the Connect call passes and then assert that it generated correct results.

@heplesser

heplesser May 4, 2016

Contributor

This test depends on the previous test to create the network. It would be better to join the previous test with this one. assert is a tighter requirement than pass, so it is not strictly necessary to test first that the Connect call passes and then assert that it generated correct results.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 4, 2016

Contributor

@janhahne 👍 from me if you remove the last inter-test dependencies. Since @jougs approved, I we will be ready to merge once you fix those.

Contributor

heplesser commented May 4, 2016

@janhahne 👍 from me if you remove the last inter-test dependencies. Since @jougs approved, I we will be ready to merge once you fix those.

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne May 4, 2016

Contributor

@heplesser Done

Contributor

janhahne commented May 4, 2016

@heplesser Done

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 4, 2016

Contributor

... and merging!

Contributor

heplesser commented May 4, 2016

... and merging!

@heplesser heplesser merged commit d32796a into nest:master May 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@janhahne janhahne deleted the janhahne:symmetric_flag branch May 4, 2016

heplesser added a commit to heplesser/nest-simulator that referenced this pull request Nov 22, 2016

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