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

Converting some manualtests to tests used by installcheck #774

Merged
merged 30 commits into from
Oct 11, 2017

Conversation

hakonsbm
Copy link
Contributor

This PR updates and converts some of the tests in the manualtests folder to proper unittests. This includes the test_sinusoidal_poisson_generator_{5,6} tests, and therefore resolves issue #348.

I was unable to convert all the tests because of problems such as missing input files, dependence on external programs, and erroneous output.

@heplesser heplesser added ZC: Infrastructure DO NOT USE THIS LABEL I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) ZP: PR Created DO NOT USE THIS LABEL S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. labels Jun 30, 2017
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hakonsbm Thanks for moving the tests! I have some minor comments, see inline. The two sinusoidal poisson tests contain a note in their documentation saying that they don't work with the testsuite. Those comments can now be deleted :).

stim = nest.Create('spike_generator')
neuronA = nest.Create('parrot_neuron')
neuronB = nest.Create('parrot_neuron')
# recorder = nest.Create('spike_detector')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unused code.

'weight': float(startWeight) / 15.0 * Wmax,
'delay': delay, 'model': modelName})
# nest.Connect(neuronA, recorder)
# nest.Connect(neuronB, recorder)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unused code.


# just before theoretical updates
weightTraceMod36pre = weightTrace[35::36]
weightTraceMod36 = weightTrace[::36] # just after theoretical updates
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment for line 123 is on line 122, the one for 124 on the same line.

self.assertLess(ratio, 1.5)

isi = []
for i in xrange(1, len(spikes)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xrange disappears in Python 3. A much more efficient way to do this would be using numpy arrays.

self.assertLess(ratio, 1.5)

isi = []
for i in xrange(1, len(spikes)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

# reproducible in NEST. Adaptive threshold changes rate, thus
# the ratio is not asserted here.
isi = []
for i in xrange(1, len(spikes)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

isi_m1 = isi[-1]
isi_m2 = isi[-1] ** 2
isi_12 = 0.
for t, t1 in zip(isi[:-1], isi[1:]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.


(unittest) run
/unittest using

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test requires GSL.

@@ -104,15 +104,10 @@ See also: test_sinusoidal_poisson_generator_{1,2,3,4,6}, test_sinusoidal_poisson
% get events, replace vectors with SLI arrays
% keep only non-empty arrays; empty ones are from off-process parrots
(ALL LINES SHOULD BE EQUAL) ==
sdets { [/events /times] get cva } Map { empty not exch ; } Select
sdets { [/events /times] get cva } Map { empty not exch ; } Select dup /spike_times Set
{ == } forall
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line just prints all the spike times, filling the log with potentially garbled data from the two processes. I would remove it; then, you don't need the dup /spike_times Set.

@@ -117,15 +117,10 @@ See also: test_sinusoidal_poisson_generator_{1,2,3,4,6}, test_sinusoidal_poisson
% get events, replace vectors with SLI arrays
% keep only non-empty arrays; empty ones are from off-process parrots
(ALL LINES SHOULD BE DIFFERENT) ==
sdets { [/events /times] get cva } Map { empty not exch ; } Select
sdets { [/events /times] get cva } Map { empty not exch ; } Select dup /sts Set
{ == } forall
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

Removed unnecessary comments, printing; using numpy functions for
efficiency; skipping if without GSL where needed.
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hakonsbm Thanks for the fixes, just a few details left.

isi_var = (isi_m2 - isi_m1) ** 2 / len(isi)
isi_mean = np.mean(isi)
isi_var = np.var(isi)
isi_12 = np.sum(np.multiply(isi[:-1], isi[1:]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isi_12 = np.sum(isi[:-1] * isi[1:])

@@ -104,10 +98,9 @@ See also: test_sinusoidal_poisson_generator_{1,2,3,4,6}, test_sinusoidal_poisson
% get events, replace vectors with SLI arrays
% keep only non-empty arrays; empty ones are from off-process parrots
(ALL LINES SHOULD BE EQUAL) ==
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This output should also be removed.

@@ -117,10 +112,8 @@ See also: test_sinusoidal_poisson_generator_{1,2,3,4,6}, test_sinusoidal_poisson
% get events, replace vectors with SLI arrays
% keep only non-empty arrays; empty ones are from off-process parrots
(ALL LINES SHOULD BE DIFFERENT) ==
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This output should also be removed.

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for the effort. I sent you a pull requests with minor changes to formatting of the comments. If those are merged, I have no objections to merging this one.

@jougs
Copy link
Contributor

jougs commented Jul 6, 2017

@heplesser, @hakonsbm: does this PR also mean that all other tests in manualtests cannot be converted to automatic tests? If yes, should we delete them or rather convert them to examples? Are you planning to work on that?

hakonsbm and others added 2 commits July 6, 2017 09:45
Minor changes to comments and formatting
@jougs
Copy link
Contributor

jougs commented Jul 6, 2017

Sorry, my commit introduced a pep8 error (see Travis log). I've sent another PR fixing this.

The current failure above, however, is not in the code touched by this PR or my changes but in the compilation of MUSIC. It happens when the make target pyconfig.pxi runs the script test.py, which tries to import mpi4py, which apparently fails with ImportError: No module named mpi4py. I don't understand how this can happen, as we install mpi4py as we always did.

Maybe a simple re-run helps...

@hakonsbm
Copy link
Contributor Author

hakonsbm commented Jul 6, 2017

@jougs: @heplesser and I just went though the rest of the tests to see which can be fixed/converted to examples and which should be deleted. @stinebuu and I will work on it.

@hakonsbm
Copy link
Contributor Author

hakonsbm commented Jul 6, 2017

My last commit message was a bit unclear. The tests deleted are tests that are now irrelevant.

@lekshmideepu
Copy link
Contributor

travis-ci/travis-ci#8048

@lekshmideepu
Copy link
Contributor

Probably we could switch back to the old images by adding the group: deprecated-2017Q2 instead of "edge"
https://blog.travis-ci.com/2017-06-21-trusty-updates-2017-Q2-launch

@jougs
Copy link
Contributor

jougs commented Jul 17, 2017

@hakonsbm: I've just merged #785, which makes our setup use the old image. Please revert 2e8d285 and merge upstream/master into this branch. Thanks!

@heplesser
Copy link
Contributor

@jougs @hakonsbm All tests pass now on Travis. Does this mean that this PR is ready to be merged?

@hakonsbm
Copy link
Contributor Author

hakonsbm commented Aug 9, 2017

@heplesser Some of the tests were missing reference values, or had other problems. @stinebuu and I are still waiting for replies from the authors of these tests, requesting guidance as to how they can be converted, or if they can be deleted.

@hakonsbm
Copy link
Contributor Author

The remaining problematic tests in the manualtests directory are now

  • cross_check_test_mip_corrdet.py
  • stdp_dopa_check.py
  • test_stdp_dopa.py

The SLI modifications are now discussed in issue nest#811.
@jougs
Copy link
Contributor

jougs commented Aug 21, 2017

@suku248, @abigailm: can you please have a look at the *dopa* tests mentioned in @hakonsbm's comment and check if they a) are still needed and b) should be converted to proper automatic tests?

@mhelias: can you please do the same for cross_check_test_mip_corrdet.py? In particular, does this test add anything on top of test_corr_det.sli in the unittests directory?

Thanks!

@suku248
Copy link
Contributor

suku248 commented Sep 5, 2017

stdp_dopa_check.py and test_stdp_dopa.py are outdated and can be removed. A proper automatic test is required.

@heplesser
Copy link
Contributor

@suku248 Could you create an issue to remind us to create proper tests for dopa?

@mhelias
Copy link
Contributor

mhelias commented Sep 8, 2017

As far as I see the
test_mip_corrdet.sli does not add anything to the existing test
test_corrdet.sli.
But it is a useful example how to use the mip generator and to
verify that it produces correlated spike trains. So I propose to move
it to examples.

@hakonsbm
Copy link
Contributor Author

@jougs The cross_check_mip_corrdet.py test has now been converted to an example, and the rest of the tests have been removed from the manualtests directory.

@heplesser heplesser added this to the NEST 2.14.0 milestone Oct 3, 2017
@heplesser
Copy link
Contributor

@jougs I added this to 2.14, since it appears done and ready to merge.

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for this important clean-up :-)

@jougs jougs merged commit 4ff76ac into nest:master Oct 11, 2017
@hakonsbm hakonsbm deleted the manualtests branch October 12, 2017 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. ZC: Infrastructure DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants