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

Fixed SetDefaults for synapse models #704

Merged
merged 2 commits into from Apr 7, 2017

Conversation

Projects
None yet
3 participants
@janhahne
Contributor

janhahne commented Apr 6, 2017

This PR fixes #545.

@heplesser

The code looks good, but could you add a regression test, preferably in SLI?

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Apr 7, 2017

Contributor

@heplesser Added a regression test, please have another look.

Contributor

janhahne commented Apr 7, 2017

@heplesser Added a regression test, please have another look.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 7, 2017

Contributor

@janhahne Looks good (and even crashes with my version of NEST), but since this is a threading-issue, it would be even better to have a multithreaded part as well. Could you add something like the following at the end of the test?

is_threaded not { exit_test_gracefully } if

{
  ResetKernel
  0 << /local_num_threads 4 >> SetStatus
  // bad SetDefaults
} fail_or_die

and may be passing one as well?

Contributor

heplesser commented Apr 7, 2017

@janhahne Looks good (and even crashes with my version of NEST), but since this is a threading-issue, it would be even better to have a multithreaded part as well. Could you add something like the following at the end of the test?

is_threaded not { exit_test_gracefully } if

{
  ResetKernel
  0 << /local_num_threads 4 >> SetStatus
  // bad SetDefaults
} fail_or_die

and may be passing one as well?

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Apr 7, 2017

Contributor

@heplesser I added the same tests executed with multiple threads.

Contributor

janhahne commented Apr 7, 2017

@heplesser I added the same tests executed with multiple threads.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Apr 7, 2017

Contributor

@flinz Could you check if this solves #545 for you?

Contributor

heplesser commented Apr 7, 2017

@flinz Could you check if this solves #545 for you?

@flinz

flinz approved these changes Apr 7, 2017

Looking good to me!
I checked locally that this solves the issue.
Thanks Jan 👍

@heplesser heplesser merged commit 54d8110 into nest:master Apr 7, 2017

1 check passed

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

@janhahne janhahne deleted the janhahne:fix_545 branch Apr 10, 2017

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