Fix 665: Connecting to layers of recording devices #666

Merged
merged 7 commits into from Mar 1, 2017

Conversation

@heplesser
Contributor

heplesser commented Feb 28, 2017

This PR addresses #665. When connecting to a layer of recording devices without proxies, i.e., with thread siblings, ConnectLayers only connected on layer 0. This is now fixed for convergent connections without prescribed number of connections, which was the approach recommended in the manual, Sec 3.10, previously. The fix uses the same technique as in ConnBuilder. For all other topology connection types, connecting to layers containing devices without proxies is now explicitly prohibited.

@terhorstd

nice work, just some more curly-braces, then it's fine.

topology/connection_creator_impl.h
if ( target_filter_.select_model()
- && ( ( *tgt_it )->get_model_id() != target_filter_.model ) )
+ && ( tgt->get_model_id() != target_filter_.model ) )
continue;

This comment has been minimized.

@terhorstd

terhorstd Feb 28, 2017

Contributor

this should also be in { }, as the one above

@terhorstd

terhorstd Feb 28, 2017

Contributor

this should also be in { }, as the one above

This comment has been minimized.

@heplesser

heplesser Feb 28, 2017

Contributor

Done.

@heplesser

heplesser Feb 28, 2017

Contributor

Done.

topology/connection_creator_impl.h
target_pos,
- target_thread,
+ thread_id,
source );
else

This comment has been minimized.

@terhorstd

terhorstd Feb 28, 2017

Contributor

since the connect_to_target call is getting kind of multi-line, adding { } to the 'if' would ease the reading a lot

@terhorstd

terhorstd Feb 28, 2017

Contributor

since the connect_to_target call is getting kind of multi-line, adding { } to the 'if' would ease the reading a lot

This comment has been minimized.

@heplesser

heplesser Feb 28, 2017

Contributor

Done.

@heplesser

heplesser Feb 28, 2017

Contributor

Done.

@jougs

Thanks! I have just a very minor comment on the code.

However, all the figures are changed and I think the new versions are problematic because of two reasons:

  • the labels are harder to read because of the decreased size
  • the contrast of gray boxes is gone in figures where the grid lines were changed from dotted gray to solid gray.

I could not see changes in the code that lead to theese changes. Did you change your matplotlib stylesheet?

+fail_or_die
+clear
+
+endusing

This comment has been minimized.

@jougs

jougs Feb 28, 2017

Contributor

Please add a newline at the end of the file.

@jougs

jougs Feb 28, 2017

Contributor

Please add a newline at the end of the file.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Feb 28, 2017

Contributor
Contributor

heplesser commented Feb 28, 2017

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Feb 28, 2017

Contributor

@jougs @terhorstd The points you raised should be fixed now.

Contributor

heplesser commented Feb 28, 2017

@jougs @terhorstd The points you raised should be fixed now.

@jougs

jougs approved these changes Feb 28, 2017

Many thanks for fixing.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Feb 28, 2017

Contributor

@heplesser: I'm afraid that the merge of #546 (which you approved) activated full VERA++ checks and now this pull request requires the addition of more curly braces in conditionals. Bad luck ;-)

Contributor

jougs commented Feb 28, 2017

@heplesser: I'm afraid that the merge of #546 (which you approved) activated full VERA++ checks and now this pull request requires the addition of more curly braces in conditionals. Bad luck ;-)

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Feb 28, 2017

Contributor

@jougs I have hopefully fixed everything now after merging the new style checker into this PR. I even found and fixed a tiny, topology-related bug in the style checker. Travis is not running properly right now due to S3 problems, so we may need to wait a little to be really sure.

Contributor

heplesser commented Feb 28, 2017

@jougs I have hopefully fixed everything now after merging the new style checker into this PR. I even found and fixed a tiny, topology-related bug in the style checker. Travis is not running properly right now due to S3 problems, so we may need to wait a little to be really sure.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Feb 28, 2017

Contributor

Thanks for the fast reaction. Do you have an idea why the Travis display disappeared from here?

Contributor

jougs commented Feb 28, 2017

Thanks for the fast reaction. Do you have an idea why the Travis display disappeared from here?

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Feb 28, 2017

Contributor

Ah, OK. I got it. Travis is running not at all due to the problems. I assume we have to wait then. Nevermind.

Contributor

jougs commented Feb 28, 2017

Ah, OK. I got it. Travis is running not at all due to the problems. I assume we have to wait then. Nevermind.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 1, 2017

Contributor

@jougs @terhorstd The tests finally passed after some more fixes to the Travis checks. Sorry for not testing #546 on Travis thoroughly enough.

Contributor

heplesser commented Mar 1, 2017

@jougs @terhorstd The tests finally passed after some more fixes to the Travis checks. Sorry for not testing #546 on Travis thoroughly enough.

@terhorstd terhorstd merged commit 5003e2b into nest:master Mar 1, 2017

1 check passed

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

@heplesser heplesser deleted the heplesser:fix-665-topoconnect branch Mar 2, 2017

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