Skip to content
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

Correction in the test of synapse disconnection with mpi #829

Merged
merged 3 commits into from Mar 7, 2018

Conversation

Projects
None yet
6 participants
@sdiazpier
Copy link
Contributor

sdiazpier commented Sep 20, 2017

This PR solves an issue with the test of synapse deletion using mpi and pynest. Thanks to @lekshmideepu who implemented the solution in PR #789 but we have decided to split this change as this problem is not directly related to the issue in that PR.

@heplesser

This comment has been minimized.

Copy link
Contributor

heplesser commented Oct 30, 2017

@sdiazpier Could you check why the tests are failing on Travis?

@hakonsbm
Copy link
Contributor

hakonsbm left a comment

Looks pretty good, but see my comment about why it fails on Travis.

Additionally:

  • iaf_neuron is deprecated. Use iaf_psc_alpha instead.
  • In line 100 assertFail() is called, but it is never defined. However Python doesn't complain because
  • There is a bare except in line 101. Please use except nest.NESTError instead.
import nest
import unittest
import numpy as np

This comment has been minimized.

@hakonsbm

hakonsbm Oct 30, 2017

Contributor

Numpy isn't used in this test.

'rate_connection_instantaneous',
'rate_connection_instantaneous_lbl',
'rate_connection_delayed',
'rate_connection_delayed_lbl'

This comment has been minimized.

@hakonsbm

hakonsbm Oct 30, 2017

Contributor

Removing these makes the test fail because these synapse-models aren't supported by iaf_neuron/iaf_psc_alpha.

This comment has been minimized.

@sanjayankur31

sanjayankur31 Oct 31, 2017

Contributor

Confirming - these need to be checked and added back to the exclude list

@sanjayankur31
Copy link
Contributor

sanjayankur31 left a comment

Looks OK mostly - @hakonsbm pointed out most of them - the exclude list needs to be corrected of course, and my flake8 linter doesn't like the space between print and the opening bracket on line 103, but that's purely a cosmetic issue :)

'rate_connection_instantaneous',
'rate_connection_instantaneous_lbl',
'rate_connection_delayed',
'rate_connection_delayed_lbl'

This comment has been minimized.

@sanjayankur31

sanjayankur31 Oct 31, 2017

Contributor

Confirming - these need to be checked and added back to the exclude list

updated with master, added missing connections to the exceptions list…
… and fixed the failure of non existent synapses
@sdiazpier

This comment has been minimized.

Copy link
Contributor Author

sdiazpier commented Feb 8, 2018

Dear @hakonsbm and @sanjayankur31 , sorry for the delay to address all these comments. I hope I have solved all of them. Please let me know if you have any additional comments or suggested changes.

@hakonsbm
Copy link
Contributor

hakonsbm left a comment

Thanks for the changes. It looks fine to me now.

@terhorstd

This comment has been minimized.

Copy link
Contributor

terhorstd commented Mar 5, 2018

@sanjayankur31 can you have another look? Thanks a lot!

@terhorstd terhorstd added this to the NEST 2.16 milestone Mar 5, 2018

@jougs jougs merged commit ef67bd7 into nest:master Mar 7, 2018

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
You can’t perform that action at this time.