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

Add tests for synapse parameter check (fixes #735) #736

Merged
merged 6 commits into from Jun 6, 2017

Conversation

Projects
None yet
3 participants
@heplesser
Contributor

heplesser commented May 29, 2017

This PR removes some code from ConnBuilder that was meant to raise an error if the user attempted to set stdp parameters on individual stdp_dopamine_synapses that could not be set individually on those synapses. I removed the entire test because:

  • the test was not used at all: if attempting to set such a parameter, e.g., /A_minus, the error was caught by the "unused parameter" test
  • the test would not have worked at all on copies of stdp_dopamine_synapse, because it looked at the synapse name to decide whether the test should be applied
  • the test was unsafe by implicitly introducing Name objects (see #722)

I added regression test to ensure that attempts to set model-only parameters on individual synapses raise errors.

@maharjun Could you check that this addresses your concerns in #735?

@maharjun

This comment has been minimized.

Show comment
Hide comment
@maharjun

maharjun May 29, 2017

Contributor

TBH I'm not really familiar with the way the connection builders work, the issue #735 was onlly noticed by me when I was inspecting the diffs, and this does indeed remove the offending piece of code. Regarding actual function, it would be better if someone else were to review that.

Contributor

maharjun commented May 29, 2017

TBH I'm not really familiar with the way the connection builders work, the issue #735 was onlly noticed by me when I was inspecting the diffs, and this does indeed remove the offending piece of code. Regarding actual function, it would be better if someone else were to review that.

@stinebuu

This comment has been minimized.

Show comment
Hide comment
@stinebuu

stinebuu May 30, 2017

Contributor

I am a bit confused about what the removed test is supposed to do. I don't see the connection between
the regression test and the removed test, as the removed test check to see if the parameters are in the syn_spec sent in with Connect and the regression test tests errors with SetStatus.

Also, when testing with master on my computer, I get the "unused parameter" error if I do

/n /iaf_psc_alpha Create def
n n << /model /stdp_dopamine_synapse /A_plus 2 >> Connect 

This call never enters ConnBuilder::check_synapse_params_, so the removed test is never tested here, as you said.

If I do

/n /iaf_psc_alpha Create def
[n] [n] << /rule /all_to_all >> << /model /stdp_dopamine_synapse /A_plus 2 >> Connect

however, I get

Connect_g_g_D_D [Error]: NotImplemented
    Connect doesn't support the setting of parameter A_plus in stdp_dopamine_synapse. Use SetDefaults() or CopyModel().

The same error occurs with /one_to_one rule, I did not check the other rules.

This disappears when the test is removed, of course. Are we not supposed to test to see if we are sending in these parameters in syn_spec for the stdp_dopamine_synapse anymore? Even though it only appears when we send in a rule?

Contributor

stinebuu commented May 30, 2017

I am a bit confused about what the removed test is supposed to do. I don't see the connection between
the regression test and the removed test, as the removed test check to see if the parameters are in the syn_spec sent in with Connect and the regression test tests errors with SetStatus.

Also, when testing with master on my computer, I get the "unused parameter" error if I do

/n /iaf_psc_alpha Create def
n n << /model /stdp_dopamine_synapse /A_plus 2 >> Connect 

This call never enters ConnBuilder::check_synapse_params_, so the removed test is never tested here, as you said.

If I do

/n /iaf_psc_alpha Create def
[n] [n] << /rule /all_to_all >> << /model /stdp_dopamine_synapse /A_plus 2 >> Connect

however, I get

Connect_g_g_D_D [Error]: NotImplemented
    Connect doesn't support the setting of parameter A_plus in stdp_dopamine_synapse. Use SetDefaults() or CopyModel().

The same error occurs with /one_to_one rule, I did not check the other rules.

This disappears when the test is removed, of course. Are we not supposed to test to see if we are sending in these parameters in syn_spec for the stdp_dopamine_synapse anymore? Even though it only appears when we send in a rule?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 30, 2017

Contributor

@stinebuu Thank you for your detective work. Clearly I need to extend the regression test to include the case with the "long" form of the Connect call. I am surprised the two forms make such a difference.

Contributor

heplesser commented May 30, 2017

@stinebuu Thank you for your detective work. Clearly I need to extend the regression test to include the case with the "long" form of the Connect call. I am surprised the two forms make such a difference.

heplesser added some commits May 30, 2017

Added parameter setting on Connect to regressiontest; re-instated che…
…ck for non-settable parameters in ConnBuilder::check_synapse_params_(). Regression test still failing when setting non-settable parameter at Connect on copy of stdp_dopamine_synpase.
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser May 30, 2017

Contributor

@stinebuu The reason that Connect in the long form triggered the error message was that check_synapse_parameters_() is called when you let Connect set synapse parameters (from the ConnBuilder constructor). I now put the test back into check_synapse_parameters_() and extended the regression test. Unfortunately, that fails. To fix this, we need to fix #737 first, so that the test will be done correctly even if we use a synapse model created with CopyModel.

Contributor

heplesser commented May 30, 2017

@stinebuu The reason that Connect in the long form triggered the error message was that check_synapse_parameters_() is called when you let Connect set synapse parameters (from the ConnBuilder constructor). I now put the test back into check_synapse_parameters_() and extended the regression test. Unfortunately, that fails. To fix this, we need to fix #737 first, so that the test will be done correctly even if we use a synapse model created with CopyModel.

@heplesser heplesser added P: Blocked and removed P: PR Created labels May 30, 2017

@stinebuu

Looks good!

@heplesser heplesser changed the title from Remove unused synapse parameter check (fixes #735) to Add tests for synapse parameter check (fixes #735) Jun 6, 2017

@heplesser heplesser merged commit 37f63bd into nest:master Jun 6, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Jun 6, 2017

Contributor

NB: Most of the work was done by #739. This PR just adds an additional test.

Contributor

heplesser commented Jun 6, 2017

NB: Most of the work was done by #739. This PR just adds an additional test.

@heplesser heplesser deleted the heplesser:fix-735-spurious-space branch Jun 6, 2017

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