Fixed get targets for structural plasticity #604

Merged
merged 11 commits into from Jan 12, 2017

Conversation

Projects
None yet
4 participants
@sdiazpier
Contributor

sdiazpier commented Dec 23, 2016

This PR addresses issue #576 and issue #589 and substitutes PRs #582 and #584 and #595 by adding a new get targets function which checks if the target has defined synaptic elements before adding them to the possible targets list for deletion. After a deep analysis, I realized that the list of targets only cares for the synapse model and not for the existence of the synaptic elements in the target node. Usually a new synapse model is created for each synaptic elements pair. However this is not enforced in any way in the code and when one uses the same synapse model for all connections, this error will arise.
Checking if the node contains the desired synaptic elements declared solves this problem.
Thank you to @sanjayankur31 and @jakobj who have been working hard to solve and understand this issue. This PR is the result of our accumulated efforts.

@sdiazpier

This comment has been minimized.

Show comment
Hide comment
@sdiazpier

sdiazpier Dec 23, 2016

Contributor

@sanjayankur31 and @jakobj could you please review this PR?

Contributor

sdiazpier commented Dec 23, 2016

@sanjayankur31 and @jakobj could you please review this PR?

sdiazpier added some commits Jan 7, 2017

Added changes to handle deletion when structural plasticity is not en…
…abled. This can be called directly by the user by calling the disconnect function. The new changes make sure that the connection exists before deleting it.
@jakobj

jakobj suggested changes Jan 8, 2017 edited

Nice work @sdiazpier! i've added a few comment, and you should fix PEP8 formatting and the copyright header of the new unittest (wrong filename, see travis log).

nestkernel/connection_manager.cpp
+nest::ConnectorBase*
+nest::ConnectionManager::validate_source_entry_( thread tid, index s_gid )

This comment has been minimized.

@jakobj

jakobj Jan 8, 2017

Contributor

these parameters could be const, same for function above.

@jakobj

jakobj Jan 8, 2017

Contributor

these parameters could be const, same for function above.

nestkernel/connection_manager.cpp
->get_target_gids( ( *target_it ), thread_id, synapse_model );
}
}
}
}
+
+void
+nest::ConnectionManager::get_targets( std::vector< index > sources,

This comment has been minimized.

@jakobj

jakobj Jan 8, 2017

Contributor

this function replaces the original get_targets function, correct? if so, you should remove the outdated function.

@jakobj

jakobj Jan 8, 2017

Contributor

this function replaces the original get_targets function, correct? if so, you should remove the outdated function.

This comment has been minimized.

@sdiazpier

sdiazpier Jan 10, 2017

Contributor

Yes, I thought it was being used somewhere else but it is not the case.

@sdiazpier

sdiazpier Jan 10, 2017

Contributor

Yes, I thought it was being used somewhere else but it is not the case.

nestkernel/connection_manager.cpp
+ targets.resize( sources.size() );
+ for ( std::vector< std::vector< index > >::iterator i = targets.begin();
+ i != targets.end();
+ i++ )

This comment has been minimized.

@jakobj

jakobj Jan 8, 2017

Contributor

++i

@jakobj

jakobj Jan 8, 2017

Contributor

++i

nestkernel/connection_manager.cpp
+ ( *i ).clear();
+ }
+
+ for ( tVSConnector::iterator it = connections_.begin();

This comment has been minimized.

@jakobj

jakobj Jan 8, 2017

Contributor

could you turn this into a loop of this sort

for ( thread tid = 0; tid < connections_.size(); ++tid )
{
  [...]
}

or are there reasons to stick to the iterators. I find it quite hard to read. this would also make it easier to introduce a threaded version later.

@jakobj

jakobj Jan 8, 2017

Contributor

could you turn this into a loop of this sort

for ( thread tid = 0; tid < connections_.size(); ++tid )
{
  [...]
}

or are there reasons to stick to the iterators. I find it quite hard to read. this would also make it easier to introduce a threaded version later.

nestkernel/connector_base.h
+ {
+ if ( get_syn_id() == synapse_id )
+ {
+ for ( size_t i = 0; i < K; i++ )

This comment has been minimized.

@jakobj

jakobj Jan 8, 2017

Contributor

++i

@jakobj

jakobj Jan 8, 2017

Contributor

++i

nestkernel/connector_base.h
@@ -1094,6 +1158,22 @@ class HetConnector : public std::vector< ConnectorBase* >, public ConnectorBase
}
+ void
+ get_target_gids( std::vector< size_t >& target_gids,

This comment has been minimized.

@jakobj

jakobj Jan 8, 2017

Contributor

(see above)

@jakobj

jakobj Jan 8, 2017

Contributor

(see above)

nestkernel/connector_base.h
+ synindex synapse_id,
+ std::string post_synaptic_element ) const
+ {
+ for ( size_t i = 0; i < size(); i++ )

This comment has been minimized.

@jakobj

jakobj Jan 8, 2017

Contributor

++i

@jakobj

jakobj Jan 8, 2017

Contributor

++i

nestkernel/sp_manager.cpp
+ disconnect( sgid,
+ target,
+ target_thread,
+ kernel().model_manager.get_synapsedict()->lookup( syn_name ) );

This comment has been minimized.

@jakobj

jakobj Jan 8, 2017

Contributor

isn't this the same code as in the if block? why did you duplicate it?

@jakobj

jakobj Jan 8, 2017

Contributor

isn't this the same code as in the if block? why did you duplicate it?

This comment has been minimized.

@sdiazpier

sdiazpier Jan 10, 2017

Contributor

Yes this is right thanks for noticing, I was moving things around and at the end remained the same code. I am correcting it now.

@sdiazpier

sdiazpier Jan 10, 2017

Contributor

Yes this is right thanks for noticing, I was moving things around and at the end remained the same code. I am correcting it now.

]
-
+

This comment has been minimized.

@jakobj

jakobj Jan 8, 2017

Contributor

please remove whitespace

@jakobj

jakobj Jan 8, 2017

Contributor

please remove whitespace

+
+try:
+ nest.Simulate(200*1000)
+except ExceptionType:

This comment has been minimized.

@jakobj

jakobj Jan 8, 2017

Contributor

i think this should just read except: if you want to catch all exceptions.

@jakobj

jakobj Jan 8, 2017

Contributor

i think this should just read except: if you want to catch all exceptions.

@sanjayankur31

Not a lot other than what @jakobj already pointed out. Thanks for working on this @sdiazpier. Should be OK to merge once the minor issues are fixed (and the build passes :))

👍

nestkernel/connection_manager.cpp
@@ -640,6 +640,22 @@ nest::ConnectionManager::disconnect( Node& target,
if ( kernel().node_manager.is_local_gid( target.get_gid() ) )
{
+ // We check that a connection actually exists between target and source
+ // This is to propperly handle the case when structural plasticity is not

This comment has been minimized.

@sanjayankur31

sanjayankur31 Jan 10, 2017

Contributor

s/propperly/properly/ :)

@sanjayankur31

sanjayankur31 Jan 10, 2017

Contributor

s/propperly/properly/ :)

nestkernel/sp_manager.cpp
@@ -217,7 +217,7 @@ SPManager::disconnect_single( index sgid,
thread target_thread,
DictionaryDatum& syn )
{
-
+ // Disconnect if Structural plascity is activated

This comment has been minimized.

@sanjayankur31

sanjayankur31 Jan 10, 2017

Contributor

s/plascity/plasticity/

@sanjayankur31

sanjayankur31 Jan 10, 2017

Contributor

s/plascity/plasticity/

@sdiazpier

This comment has been minimized.

Show comment
Hide comment
@sdiazpier

sdiazpier Jan 10, 2017

Contributor

@sanjayankur31 and @jakobj thanks for your comments. I have addressed them and fixed the formatting. Please let me know if you think all issues have been solved.

Contributor

sdiazpier commented Jan 10, 2017

@sanjayankur31 and @jakobj thanks for your comments. I have addressed them and fixed the formatting. Please let me know if you think all issues have been solved.

@sanjayankur31

Thanks for the updates @sdiazpier - just a few cosmetic changes before we merge :)

nestkernel/connection_manager.cpp
it != connections_.end();
- ++it )
+ ++it )*/

This comment has been minimized.

@sanjayankur31

sanjayankur31 Jan 11, 2017

Contributor

Cosmetic change - remove the commented code. It doesn't contribute to the readability much here. If you think a comment describing the logic here is needed, maybe you can add a descriptive comment instead?

@sanjayankur31

sanjayankur31 Jan 11, 2017

Contributor

Cosmetic change - remove the commented code. It doesn't contribute to the readability much here. If you think a comment describing the logic here is needed, maybe you can add a descriptive comment instead?

nestkernel/connection_manager.cpp
{
- thread_id = it - connections_.begin();
+ // thread_id = it - connections_.begin();

This comment has been minimized.

@sanjayankur31

sanjayankur31 Jan 11, 2017

Contributor

Same cosmetic change as above - the commented code isn't needed and a descriptive comment will fit better if you think it's needed here.

@sanjayankur31

sanjayankur31 Jan 11, 2017

Contributor

Same cosmetic change as above - the commented code isn't needed and a descriptive comment will fit better if you think it's needed here.

+ try:
+ nest.Simulate(200 * 1000)
+ except:
+ self.fail("Exception during structural plasticity deletion")

This comment has been minimized.

@sanjayankur31

sanjayankur31 Jan 11, 2017

Contributor

The test isn't only forcing deletion of synapses - all exceptions are being caught too, so the exception error message should be made more general. What do you think?

@sanjayankur31

sanjayankur31 Jan 11, 2017

Contributor

The test isn't only forcing deletion of synapses - all exceptions are being caught too, so the exception error message should be made more general. What do you think?

@sdiazpier

This comment has been minimized.

Show comment
Hide comment
@sdiazpier

sdiazpier Jan 11, 2017

Contributor

@sanjayankur31 thanks for the comments. I have removed the comments and I hope it is all good. Please let me know if anything is missing.

Contributor

sdiazpier commented Jan 11, 2017

@sanjayankur31 thanks for the comments. I have removed the comments and I hope it is all good. Please let me know if anything is missing.

@sanjayankur31

LGTM 👍 :)

@jakobj

jakobj approved these changes Jan 11, 2017 edited

Just a small comment. And the typos @sanjayankur31 pointed out should also be corrected. Since travis is happy 👍 from me. nice work @sdiazpier !

nestkernel/connection_manager.cpp
{
- thread thread_id;
+ unsigned int thread_id;

This comment has been minimized.

@jakobj

jakobj Jan 11, 2017

Contributor

you can define this variable in line 1335, and type should be thread. I would prefer homogeneous usage of tid instead of thread_id but maybe this should be part of a maintanance PR, not this one.

@jakobj

jakobj Jan 11, 2017

Contributor

you can define this variable in line 1335, and type should be thread. I would prefer homogeneous usage of tid instead of thread_id but maybe this should be part of a maintanance PR, not this one.

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

I agree with @jakobj

@heplesser

heplesser Jan 11, 2017

Contributor

I agree with @jakobj

@heplesser

@sdiazpier Nice work, but I added a few suggestions following up on @jakobj's last comment.

nestkernel/connection_manager.cpp
@@ -1288,33 +1315,35 @@ nest::ConnectionManager::get_sources( std::vector< index > targets,
void
nest::ConnectionManager::get_targets( std::vector< index > sources,

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

Since sources is not modified inside the method, this should be const std::vector< index>& sources to avoid unnecessary copying.

@heplesser

heplesser Jan 11, 2017

Contributor

Since sources is not modified inside the method, this should be const std::vector< index>& sources to avoid unnecessary copying.

nestkernel/connection_manager.cpp
{
- thread thread_id;
+ unsigned int thread_id;

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

I agree with @jakobj

@heplesser

heplesser Jan 11, 2017

Contributor

I agree with @jakobj

nestkernel/connection_manager.cpp
{
- thread thread_id;
+ unsigned int thread_id;
std::vector< index >::iterator source_it;
std::vector< std::vector< index > >::iterator target_it;

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

I would also move these two declarations down to lines 1338, 1339.

@heplesser

heplesser Jan 11, 2017

Contributor

I would also move these two declarations down to lines 1338, 1339.

nestkernel/connection_manager.cpp
std::vector< index >::iterator source_it;
std::vector< std::vector< index > >::iterator target_it;
targets.resize( sources.size() );
for ( std::vector< std::vector< index > >::iterator i = targets.begin();
i != targets.end();
- i++ )
+ ++i )
{
( *i ).clear();
}

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

I think the entire code from targets.resize() can be simplified. If I understand the code right, in the end targets should be a vector of sources.size() empty vectors. I think the idiomatic way to achieve this is

std::vector< std::vector< index > >( sources.size() ).swap( targets );
@heplesser

heplesser Jan 11, 2017

Contributor

I think the entire code from targets.resize() can be simplified. If I understand the code right, in the end targets should be a vector of sources.size() empty vectors. I think the idiomatic way to achieve this is

std::vector< std::vector< index > >( sources.size() ).swap( targets );
nestkernel/connection_manager.cpp
{
- thread_id = it - connections_.begin();
// loop over the targets/sources
source_it = sources.begin();
target_it = targets.begin();
for ( ; source_it != sources.end(); source_it++, target_it++ )

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

++source_it, ++target_it is more efficient.

@heplesser

heplesser Jan 11, 2017

Contributor

++source_it, ++target_it is more efficient.

nestkernel/connection_manager.cpp
- ->get_target_gids( ( *target_it ), thread_id, synapse_model );
+ validate_pointer( validate_source_entry_( thread_id, *source_it ) )
+ ->get_target_gids(
+ ( *target_it ), thread_id, synapse_model, post_synaptic_element );

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

I think the () around *target_it are superfluous.

@heplesser

heplesser Jan 11, 2017

Contributor

I think the () around *target_it are superfluous.

nestkernel/connection_manager.cpp
{
- validate_pointer( ( *it ).get( *source_it ) )
- ->get_target_gids( ( *target_it ), thread_id, synapse_model );
+ validate_pointer( validate_source_entry_( thread_id, *source_it ) )

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

Here you call validate_source_entry_() for a second time with the same arguments. Perhaps better (guessing that ConnectorBase is the right type here

ConnectorBase const * const source = validate_source_entry_( thread_id, *source_it );
if ( source_ptr != 0 )
{
  source->get_target_gids( *target_it, thread_id, synapse_model );
}

Note that I have here assumed that the validate_pointer() call is integrated in validate_source_entry_(); that would make code here more readable, but if it has side-effects elsewhere, it may mean too much work for now.

Finally, I think the code would become more readable if you changed target_it to targets_it, since this is an iterator pointing to a vector.

@heplesser

heplesser Jan 11, 2017

Contributor

Here you call validate_source_entry_() for a second time with the same arguments. Perhaps better (guessing that ConnectorBase is the right type here

ConnectorBase const * const source = validate_source_entry_( thread_id, *source_it );
if ( source_ptr != 0 )
{
  source->get_target_gids( *target_it, thread_id, synapse_model );
}

Note that I have here assumed that the validate_pointer() call is integrated in validate_source_entry_(); that would make code here more readable, but if it has side-effects elsewhere, it may mean too much work for now.

Finally, I think the code would become more readable if you changed target_it to targets_it, since this is an iterator pointing to a vector.

@sdiazpier

This comment has been minimized.

Show comment
Hide comment
@sdiazpier

sdiazpier Jan 11, 2017

Contributor

Dear @heplesser and @jakobj thank you for your suggestions. I have now addressed your last comments. Please let me know if I missed anything.

Contributor

sdiazpier commented Jan 11, 2017

Dear @heplesser and @jakobj thank you for your suggestions. I have now addressed your last comments. Please let me know if I missed anything.

@heplesser

Thanks! There are a few tiny details left, and the two spelling errors in comments that @sanjayankur31 pointed out (connection_manager.cpp, sp_manager.cpp).

nestkernel/connection_manager.cpp
@@ -1313,37 +1313,32 @@ nest::ConnectionManager::get_sources( std::vector< index > targets,
}
void
-nest::ConnectionManager::get_targets( std::vector< index > sources,
+nest::ConnectionManager::get_targets( const std::vector< index > sources,

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

The & is missing to make it a reference.

@heplesser

heplesser Jan 11, 2017

Contributor

The & is missing to make it a reference.

nestkernel/connection_manager.cpp
- index synapse_model,
- std::string post_synaptic_element )
+ const index synapse_model,
+ const std::string post_synaptic_element )

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

Should also be a reference, const std::string& to avoid copying.

@heplesser

heplesser Jan 11, 2017

Contributor

Should also be a reference, const std::string& to avoid copying.

nestkernel/connection_manager.h
@@ -231,10 +231,10 @@ class ConnectionManager : public ManagerInterface
std::vector< std::vector< index > >& sources,
index synapse_model );
- void get_targets( std::vector< index > sources,
+ void get_targets( const std::vector< index > sources,

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

See above.

@heplesser

heplesser Jan 11, 2017

Contributor

See above.

nestkernel/connection_manager.h
- index synapse_model,
- std::string post_synaptic_element );
+ const index synapse_model,
+ const std::string post_synaptic_element );

This comment has been minimized.

@heplesser

heplesser Jan 11, 2017

Contributor

See above.

@heplesser

heplesser Jan 11, 2017

Contributor

See above.

@sdiazpier

This comment has been minimized.

Show comment
Hide comment
@sdiazpier

sdiazpier Jan 12, 2017

Contributor

Dear @heplesser I have solved the last errors in spelling and references. Please let me know if anything else is missing.

Contributor

sdiazpier commented Jan 12, 2017

Dear @heplesser I have solved the last errors in spelling and references. Please let me know if anything else is missing.

@heplesser heplesser merged commit 32de331 into nest:master Jan 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment