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

Added test_multiarea_model_siegert_rates #1153

Closed
wants to merge 1 commit into from

Conversation

AlexVanMeegen
Copy link
Contributor

Added a test that uses the Siegert neurons to compute stationary rates of the multi area model (see Schuecker et al. 2017) as requested by @terhorstd. Motivated by the finding that, before PR #1059, different ways of compiling NEST with different conda environments lead to numerically inconsistent results - see issue #210.

Three pickled numpy arrays are necessary: indegrees, weights and stationary rates of the multiarea model. For the time being, they are in a subfolder test_multiarea_model_siegert_rates/*.npy - is this the correct location?

@heplesser heplesser added ZC: Model 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: Enhancement New functionality, model or documentation labels Mar 26, 2019
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.

@AlexVanMeegen Thank you for the test. It fails on Travis since the *.npy files cannot be found, they are probably not properly installed. I am also wary about tests depending on large external data files. Would it be feasibly to

  • reduce the reference data to, say every 100th data point (or even fewer)
  • store the reference data explicitly in the test file
  • include in this test file the code that generated the reference data with detailed information on which system the reference data was created?

In that way the test would be entirely self-contained. Do you have a setup on which the test currently fails to test the test?

@AlexVanMeegen
Copy link
Contributor Author

Dear @heplesser, thanks for reviewing this. However, I am not sure if I understand your suggestions correctly.

What do you refer to with 'reference data' - the resulting mean firing rates (mf_rates.npy) or the rates and the full connectivity (mf_rates.npy, J.npy, K.npy)? Storing K.npy or J.npy in the test file would mean to store 254x255 floats, for mf_rates.npy it's 254 floats.

Also, I am not sure what you mean with 'the code that generated the reference data'. Essentially, the test is a piece of the multiarea model code that is cut out and adapted: all pre-processing (generating the connectivity) is left out and replaced by K.npy and J.npy. To get rid of these would mean to add the full multiarea model as a dependency as far as I can see.

Finally, since #1059 I don't have a setup to test the test anymore. I discussed this with @terhorstd and he suggested to include it anyways because the calculation was useful in the past to detect inconsistencies in the environment. @terhorstd, please correct me if there is a deeper motivation that I missed here.

@terhorstd
Copy link
Contributor

Yes, I do think it's a good idea to keep this mechanism around, but as discussed we would rather seek for a test with an actual single library call, than include a megabyte of data. I have opened an issue #1164 for this. This PR still contains all files and changes necessary to reproduce the behaviour (even though probably only given #1059 is reverted).

So, as we agreed, I close this PR for now.

@terhorstd terhorstd closed this Apr 8, 2019
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: Enhancement New functionality, model or documentation ZC: Model 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.

3 participants