Implemented requires_symmetric flag for synapse models #524

Merged
merged 17 commits into from Nov 23, 2016

Conversation

Projects
None yet
3 participants
@janhahne
Contributor

janhahne commented Oct 19, 2016

This PR is an addition to PR #315 and fixes issue #440. It adds an requires_symmetric flag to the ConnetorModel class. This way synapse models can be registered as two-way connections that require symmetric connections (1->2 and 2->1). If a connection of such a synapse model is created without setting the symmetric flag of the connection dicitonary to true an exception is thrown. All-to-all connections can be created without setting symmetric to true as they are symmetric by definition (and we anyway do not allow symmetric all-to-all connections for good reasons).

So far gap junctions are the only two-way connections that use this new flag.

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Oct 19, 2016

Contributor

test_labeled_synapses.py fails, as it creates non-symmetric one-to-one gap-junction connections. Not sure about the best fix for this. Gap junctions probably should not be excluded from this test. Can anyone have a look at this?

Contributor

janhahne commented Oct 19, 2016

test_labeled_synapses.py fails, as it creates non-symmetric one-to-one gap-junction connections. Not sure about the best fix for this. Gap junctions probably should not be excluded from this test. Can anyone have a look at this?

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Oct 19, 2016

Contributor

I have a question and a possible solution.

Shouldn't the requires_symmetric flag be also included in the status dictionary of the synapse such that users can find out about its value using GetDefaults and GetStatus?

If that was the case, you could just query the model in the conflicting test and add 'symmetric': True|False to the rule dictionary depending on the value of that property.

Contributor

jougs commented Oct 19, 2016

I have a question and a possible solution.

Shouldn't the requires_symmetric flag be also included in the status dictionary of the synapse such that users can find out about its value using GetDefaults and GetStatus?

If that was the case, you could just query the model in the conflicting test and add 'symmetric': True|False to the rule dictionary depending on the value of that property.

@jougs

jougs approved these changes Oct 20, 2016

Nice solution!
The remaining problems just seem to be PEP8 errors. I am happy to merge once those are fixed.

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Oct 20, 2016

Contributor

@jougs Thank you for the suggestion of this solution! Hopefully the PEP8 errors are gone with the latest commit.

Contributor

janhahne commented Oct 20, 2016

@jougs Thank you for the suggestion of this solution! Hopefully the PEP8 errors are gone with the latest commit.

@heplesser

@janhahne I added some comments, could you take a look?

@jougs Please wait for Jan's responses before merging.

nestkernel/conn_builder.cpp
@@ -382,9 +382,18 @@ nest::ConnBuilder::change_connected_synaptic_elements( index sgid,
void
nest::ConnBuilder::connect()
{
+ if ( kernel().model_manager.connector_requires_symmetric( synapse_model_ )
+ && not symmetric_ && not is_symmetric() )

This comment has been minimized.

@heplesser

heplesser Oct 24, 2016

Contributor

@janhahne It seems to me we get a bit of an inflation here: symmetric_, is_symmetric() and supports_symmetric(). Could we at least integrate symmetric_ and is_symmetric()?

@heplesser

heplesser Oct 24, 2016

Contributor

@janhahne It seems to me we get a bit of an inflation here: symmetric_, is_symmetric() and supports_symmetric(). Could we at least integrate symmetric_ and is_symmetric()?

This comment has been minimized.

@janhahne

janhahne Oct 26, 2016

Contributor

@heplesser I agree that this is a bit unfortunate, but all three of them serve a purpose:

  • symmetric_ is the property of the created connection (is symmetric set to true or false?)
  • supports_symmetric determines if a specific ConnBuilder supports the creation of symmetric connection. This is only implemented for the OnetoOne builder.
  • is_symmetric is needed to allow all-to-all connections without setting symmetric to true. Otherwise the creation of all-to-all connection would not be possible at all for gap-junctions (all-to-all does not support symmetric=true)

Unless we want to prohibit any kind of all-to-all connections for gap-junctions I do not see any way of reducing the number of symmetric parameters. Or is there any solution that I am not thinking of?

@janhahne

janhahne Oct 26, 2016

Contributor

@heplesser I agree that this is a bit unfortunate, but all three of them serve a purpose:

  • symmetric_ is the property of the created connection (is symmetric set to true or false?)
  • supports_symmetric determines if a specific ConnBuilder supports the creation of symmetric connection. This is only implemented for the OnetoOne builder.
  • is_symmetric is needed to allow all-to-all connections without setting symmetric to true. Otherwise the creation of all-to-all connection would not be possible at all for gap-junctions (all-to-all does not support symmetric=true)

Unless we want to prohibit any kind of all-to-all connections for gap-junctions I do not see any way of reducing the number of symmetric parameters. Or is there any solution that I am not thinking of?

This comment has been minimized.

@heplesser

heplesser Oct 26, 2016

Contributor

@janhahne I wonder if we should take a step back and think this through once more. All-to-all is inherently symmetric, provided source and target populations are identical and all synapses are created with the same parameters. Guaranteeing symmetric connections when randomized parameters are used would be very difficult, and for explicit parameter arrays it would require very cumbersome checks for matrix symmetry.

So I would think the best approach might be to prohibit both array and random parameters if the synapse model is derived from gap_junction, and to also require that source and target populations are identical. Then, you get symmetry for free. @jougs Is it possible to compare two GIDCollections for equality?

@heplesser

heplesser Oct 26, 2016

Contributor

@janhahne I wonder if we should take a step back and think this through once more. All-to-all is inherently symmetric, provided source and target populations are identical and all synapses are created with the same parameters. Guaranteeing symmetric connections when randomized parameters are used would be very difficult, and for explicit parameter arrays it would require very cumbersome checks for matrix symmetry.

So I would think the best approach might be to prohibit both array and random parameters if the synapse model is derived from gap_junction, and to also require that source and target populations are identical. Then, you get symmetry for free. @jougs Is it possible to compare two GIDCollections for equality?

This comment has been minimized.

@janhahne

janhahne Oct 31, 2016

Contributor

@heplesser Good points! I think now we found all conditions that need to be checked in order to ensure that an all-to-all connection is symmetric. I would also vote for a solution where we prohibit both array and random parameters, rather than allowing those in general and check them for symmetry case by case.

So I would imagine our solution like this: Change the is_symmetric() function of the all-to-all ConnBuilder to

bool
is_symmetric() const
{
  if( sources != targets )
    return false;

  if( parameters are arrays or randomized )
    return false;

  return true;
}

and adjust the BadProperty-Message to "This synapse model requires symmetric (or suitable uniform all-to-all) connections"

The check in is_symmetric() will only be performed, if the first two conditions of the if-statement are met, ergo only if a synapse model requires symmetric connections and the symmetric flag is set to false

Would you agree on this general plan? In this case we would need to figure out how to implement the two if statements. @jougs could help with this first one, any idea how to check for the second one most efficiently?

@janhahne

janhahne Oct 31, 2016

Contributor

@heplesser Good points! I think now we found all conditions that need to be checked in order to ensure that an all-to-all connection is symmetric. I would also vote for a solution where we prohibit both array and random parameters, rather than allowing those in general and check them for symmetry case by case.

So I would imagine our solution like this: Change the is_symmetric() function of the all-to-all ConnBuilder to

bool
is_symmetric() const
{
  if( sources != targets )
    return false;

  if( parameters are arrays or randomized )
    return false;

  return true;
}

and adjust the BadProperty-Message to "This synapse model requires symmetric (or suitable uniform all-to-all) connections"

The check in is_symmetric() will only be performed, if the first two conditions of the if-statement are met, ergo only if a synapse model requires symmetric connections and the symmetric flag is set to false

Would you agree on this general plan? In this case we would need to figure out how to implement the two if statements. @jougs could help with this first one, any idea how to check for the second one most efficiently?

This comment has been minimized.

@heplesser

heplesser Oct 31, 2016

Contributor

@janhahne @jougs This looks good, although I think that we could simplify the method to

inline bool
is_symmetric() const
{
  return sources == targets && all_parameters_scalar();
}

The other method would be something like

bool
ConnBuilder::all_parameters_scalar_() const
{
  bool all_scalar = weight_->is_scalar() && delay_->is_scalar();
  for ( ConnParameterMap::const_iterator it = synapse_params_.begin() ; ... ; ++it )
  {
     all_scalar = all_scalar && it->second->is_scalar();
  }
  return all_scalar;
}

We then need in ConnParameter and extra method is_scalar() that returns true only for Scalar{Double,Integer}Parameter. We cannot use existing methods since is_array() returns false for random numbers and number_of_values() returns 0 for scalar and random.

@heplesser

heplesser Oct 31, 2016

Contributor

@janhahne @jougs This looks good, although I think that we could simplify the method to

inline bool
is_symmetric() const
{
  return sources == targets && all_parameters_scalar();
}

The other method would be something like

bool
ConnBuilder::all_parameters_scalar_() const
{
  bool all_scalar = weight_->is_scalar() && delay_->is_scalar();
  for ( ConnParameterMap::const_iterator it = synapse_params_.begin() ; ... ; ++it )
  {
     all_scalar = all_scalar && it->second->is_scalar();
  }
  return all_scalar;
}

We then need in ConnParameter and extra method is_scalar() that returns true only for Scalar{Double,Integer}Parameter. We cannot use existing methods since is_array() returns false for random numbers and number_of_values() returns 0 for scalar and random.

nestkernel/conn_builder.h
+ is_symmetric() const
+ {
+ return true;
+ }

This comment has been minimized.

@heplesser

heplesser Oct 24, 2016

Contributor

I think this is not always true. If all-to-all has a randomized synapse parameter, it is no longer automatically symmetric. We will still have a backward connection for each forward connection, but their properties will differ.

@heplesser

heplesser Oct 24, 2016

Contributor

I think this is not always true. If all-to-all has a randomized synapse parameter, it is no longer automatically symmetric. We will still have a backward connection for each forward connection, but their properties will differ.

This comment has been minimized.

@janhahne

janhahne Oct 26, 2016

Contributor

@heplesser Good catch, I did not think of that. Is there any easy way to determine if all-to-all uses randomized synapse parameters? I am thinking of a solution where this functions first checks if there are any randomized parameters involved (if so throws an BadProperty) and (otherwise) then returns true.

@janhahne

janhahne Oct 26, 2016

Contributor

@heplesser Good catch, I did not think of that. Is there any easy way to determine if all-to-all uses randomized synapse parameters? I am thinking of a solution where this functions first checks if there are any randomized parameters involved (if so throws an BadProperty) and (otherwise) then returns true.

This comment has been minimized.

@heplesser

heplesser Oct 26, 2016

Contributor

See above, array parameters are also problematic because costly to check.

@heplesser

heplesser Oct 26, 2016

Contributor

See above, array parameters are also problematic because costly to check.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Oct 26, 2016

Contributor

@heplesser: currently you can't compare GIDCollections, but that should be easy to implement. I can probably look into this next week.

Contributor

jougs commented Oct 26, 2016

@heplesser: currently you can't compare GIDCollections, but that should be easy to implement. I can probably look into this next week.

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Nov 2, 2016

Contributor

@heplesser @jougs Implemented our solution. Added tests to the symmetric_connection unittest. For some reason the comparison of the GIDCollections works fine already?

I would consider this PR as complete now, please have a look

Contributor

janhahne commented Nov 2, 2016

@heplesser @jougs Implemented our solution. Added tests to the symmetric_connection unittest. For some reason the comparison of the GIDCollections works fine already?

I would consider this PR as complete now, please have a look

@heplesser

@janhahne Very close now :). I would suggest to rename the symmetric flag at the SLI and C++ level to make_symmetric. I think that makes much clearer what this flag is about: forcing one-to-one to add connections in the opposite direction. Based on that I also suggested some changes to the symmetry test code for better readability. What do you think?

+ /set2 3 4 cvgidcollection def
+} def
+
+% Check that connections that require_symmetric cannot be created without symmetric=true

This comment has been minimized.

@heplesser

heplesser Nov 21, 2016

Contributor

Shouldn't there be one test checking that one-to-one Connect with gap_junction actually creates the expected connections? Or is that elsewhere? All further tests are just fail/pass, but do not check the result.

@heplesser

heplesser Nov 21, 2016

Contributor

Shouldn't there be one test checking that one-to-one Connect with gap_junction actually creates the expected connections? Or is that elsewhere? All further tests are just fail/pass, but do not check the result.

This comment has been minimized.

@janhahne

janhahne Nov 23, 2016

Contributor

Tests that "symmetric"/"make_symmetric" works properly with one-to-one were already included in this test (see some lines above in the same test). The new tests just concern "requires_symmetric" and the only thing to test here is if connection can or cannot be created. The functionality of the created connections is unchanged. Or what am I missing here?

@janhahne

janhahne Nov 23, 2016

Contributor

Tests that "symmetric"/"make_symmetric" works properly with one-to-one were already included in this test (see some lines above in the same test). The new tests just concern "requires_symmetric" and the only thing to test here is if connection can or cannot be created. The functionality of the created connections is unchanged. Or what am I missing here?

This comment has been minimized.

@heplesser

heplesser Nov 23, 2016

Contributor

@janhahne Sorry, my oversight!

@heplesser

heplesser Nov 23, 2016

Contributor

@janhahne Sorry, my oversight!

@@ -29,14 +29,16 @@ Synopsis: (test_symmetric_connections) run -> NEST exits if test fails
Description:
This test ensures that the functionality to create symmetric connections
-works properly.
+works properly. It also ensures that the flag requires_symmetric works properly.

This comment has been minimized.

@heplesser

heplesser Nov 21, 2016

Contributor

This is confusing, since requires_symmetric is built-in property of synapse models, not a flag visible on the SLI level. On the other hand, we have the /symmetric flag very visible in this test.

@heplesser

heplesser Nov 21, 2016

Contributor

This is confusing, since requires_symmetric is built-in property of synapse models, not a flag visible on the SLI level. On the other hand, we have the /symmetric flag very visible in this test.

nestkernel/conn_builder.cpp
@@ -386,7 +386,8 @@ nest::ConnBuilder::connect()
&& not symmetric_ && not is_symmetric() )
{
throw BadProperty(
- "This synapse model requires symmetric (or all-to-all) connections" );
+ "This synapse model requires symmetric (or suitable uniform "
+ "all-to-all) connections" );

This comment has been minimized.

@heplesser

heplesser Nov 21, 2016

Contributor

"suitable" is unclear. How about "Connections with synapse model {0} can only be created as one-to-one connections with "make_symmetric" set to true or as all-to-all connections with equal source and target populations and default or scalar parameters."?

@heplesser

heplesser Nov 21, 2016

Contributor

"suitable" is unclear. How about "Connections with synapse model {0} can only be created as one-to-one connections with "make_symmetric" set to true or as all-to-all connections with equal source and target populations and default or scalar parameters."?

nestkernel/conn_builder.cpp
@@ -386,7 +386,8 @@ nest::ConnBuilder::connect()
&& not symmetric_ && not is_symmetric() )

This comment has been minimized.

@heplesser

heplesser Nov 21, 2016

Contributor

I find this still confusing, and I fear anyone reading this in five years time will not feel enlightened. How about renaming symmetric_ to make_symmetric_ and turning the order of checks a little:

if ( kernel().model_manager.connector_requires_symmetric( synapse_model_ )
     and not ( is_symmetric() or make_symmetric_ ) )

That seems more readable to me.

@heplesser

heplesser Nov 21, 2016

Contributor

I find this still confusing, and I fear anyone reading this in five years time will not feel enlightened. How about renaming symmetric_ to make_symmetric_ and turning the order of checks a little:

if ( kernel().model_manager.connector_requires_symmetric( synapse_model_ )
     and not ( is_symmetric() or make_symmetric_ ) )

That seems more readable to me.

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Nov 23, 2016

Contributor

@heplesser I agree, make_symmetric is less confusing. Addressed all your change requests. Please have another look

Contributor

janhahne commented Nov 23, 2016

@heplesser I agree, make_symmetric is less confusing. Addressed all your change requests. Please have another look

@heplesser

@janhahne All fine, except for the one Python line that is a little too long. Once Travis is happy with that, this one is ready to merge. 👍

# try set a label during SetDefaults
with self.assertRaises(nest.NESTError):
nest.SetDefaults(syn, {'synapse_label': 123})
# try set on connect
with self.assertRaises(nest.NESTError):
- nest.Connect(a, a, {"rule": "one_to_one"}, {
- "model": syn, "synapse_label": 123})
+ nest.Connect(a, a, {"rule": "one_to_one", "make_symmetric": symm},

This comment has been minimized.

@heplesser

heplesser Nov 23, 2016

Contributor

This line is a little too long according to Travis/PEP8.

@heplesser

heplesser Nov 23, 2016

Contributor

This line is a little too long according to Travis/PEP8.

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Nov 23, 2016

Contributor

@heplesser Should be fixed now. Travis ended one test after 18s with error Failed to fetch http://security.ubuntu.com/ubuntu/dists/trusty-security/main/source/Sources. That is the reason for the remaining failed test...

Contributor

janhahne commented Nov 23, 2016

@heplesser Should be fixed now. Travis ended one test after 18s with error Failed to fetch http://security.ubuntu.com/ubuntu/dists/trusty-security/main/source/Sources. That is the reason for the remaining failed test...

@heplesser heplesser merged commit 93deb17 into nest:master Nov 23, 2016

1 check passed

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

@janhahne janhahne deleted the janhahne:symmetric_warning branch Nov 23, 2016

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