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

Fix conversion from heterogeneous to homogeneous connector when deleting last synapse of a type #566

Merged
merged 1 commit into from Dec 12, 2016

Conversation

Projects
None yet
4 participants
@jakobj
Contributor

jakobj commented Nov 25, 2016

See #565 for a discussion of the issue. This PR fixes that issue and provides a test.
I suggest @sdiazpier and @suku248 as reviewers.

@janhahne

I agree with your analysis in issue #565 and this PR is a good solution to the problem. I only have a couple of minor comments regarding the new unit test. Once they are addressed I am happy 👍

Thanks a lot for fixing that!

Show outdated Hide outdated testsuite/unittests/issue-565.sli
Synopsis: (issue-565) run -> NEST exits if test fails
Description:
This test provokes a switch from a heterogeneous connector to a homogeneous connector using the structural plasticity framework and asserts that the resulting homogeneous connector is correctly used.

This comment has been minimized.

@janhahne

janhahne Dec 1, 2016

Contributor

This line could use a line break

@janhahne

janhahne Dec 1, 2016

Contributor

This line could use a line break

Show outdated Hide outdated testsuite/unittests/issue-565.sli
% Delete the first connection again, causing the heterogeneous
% connector to be turned into a homogeneous connector
[2] cvgidcollection [1] cvgidcollection << /rule /all_to_all >> << /model /static_synapse >> Disconnect_g_g_D_D

This comment has been minimized.

@janhahne

janhahne Dec 1, 2016

Contributor

Same here, maybe add a line break after /all_to_all >>

@janhahne

janhahne Dec 1, 2016

Contributor

Same here, maybe add a line break after /all_to_all >>

Show outdated Hide outdated testsuite/unittests/issue-565.sli
% The spike detector should have recorded three events
{
3 GetStatus /n_events get 3 eq
} assert_or_die

This comment has been minimized.

@janhahne

janhahne Dec 1, 2016

Contributor

I see from your example in the issue #565 how this ensures that everything works properly, but wouldn't it be much more fail-safe to add another simulation to this unit test where one neuron with the same parameters is just connected to the spike detector (without disconnecting a temporary connection) and then (after both simulations) to compare the actual spike times recorded by the spike detector?

@janhahne

janhahne Dec 1, 2016

Contributor

I see from your example in the issue #565 how this ensures that everything works properly, but wouldn't it be much more fail-safe to add another simulation to this unit test where one neuron with the same parameters is just connected to the spike detector (without disconnecting a temporary connection) and then (after both simulations) to compare the actual spike times recorded by the spike detector?

Show outdated Hide outdated testsuite/unittests/issue-565.sli
M_ERROR setverbosity
% Create neurons and spike detector
/iaf_neuron 2 << /I_e 400. >> Create /neurons Set

This comment has been minimized.

@janhahne

janhahne Dec 1, 2016

Contributor

Use iaf_psc_alpha instead (see PR #526)

@janhahne

janhahne Dec 1, 2016

Contributor

Use iaf_psc_alpha instead (see PR #526)

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj Dec 11, 2016

Contributor

Thanks @janhahne for the quick review. I have pushed a new version addressing all your comments.

Contributor

jakobj commented Dec 11, 2016

Thanks @janhahne for the quick review. I have pushed a new version addressing all your comments.

@sdiazpier

This comment has been minimized.

Show comment
Hide comment
@sdiazpier

sdiazpier Dec 11, 2016

Contributor

Dear @jakobj thanks for fixing this issue. I agree with the solution 👍 from my side.

Contributor

sdiazpier commented Dec 11, 2016

Dear @jakobj thanks for fixing this issue. I agree with the solution 👍 from my side.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Dec 12, 2016

Contributor

@janhahne Could you confirm that @jakobj has addressed your concerns successfully?

Contributor

heplesser commented Dec 12, 2016

@janhahne Could you confirm that @jakobj has addressed your concerns successfully?

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Dec 12, 2016

Contributor

@heplesser @jakobj Yes, he did! Nice work 👍

Contributor

janhahne commented Dec 12, 2016

@heplesser @jakobj Yes, he did! Nice work 👍

@heplesser heplesser merged commit d87a66a into nest:master Dec 12, 2016

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