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

Fix quantal stdp test for numpy #681

Merged
merged 2 commits into from Mar 30, 2017

Conversation

@mschmidt87
Copy link

mschmidt87 commented Mar 15, 2017

This pull request addresses #680 and contains the following changes:

  • Convert the float variables in the test to integers when they are used as indices to numpy arrays. I decided to do this rather then defining them as integers in the first place because a) defining times as floats seems plausible to me and b) the conversion of t_tot had been implemented in this test at a different location earlier.
  • Remove two parameter dictionaries that were not used in the script.
  • Make travis download the latest release of numpy via pip.
@mschmidt87 mschmidt87 force-pushed the mschmidt87:fix_quantal_stdp_test_for_numpy branch from 1e3cf60 to 84b1252 Mar 15, 2017
Copy link
Contributor

heplesser left a comment

@mschmidt87 Please do not change the travis.yml file in this PR. I will comment on this in the issue, but it is a separate issue and requires careful consideration. I also added some small suggestions to the test.

vm.shape = (n_trials, t_tot)
vm_reference.shape = (n_trials, t_tot)
vm.shape = (n_trials, int(t_tot))
vm_reference.shape = (n_trials, int(t_tot))

This comment has been minimized.

@heplesser

heplesser Mar 16, 2017 Contributor

Using int(t_tot) here to dimension the result array is not a good idea, since you implicitly require that the voltmeter records at 1 ms intervals. A cleaner implementation would be (not 100% sure of spelling of assertions)

self.assert_equal(len(vm) % n_trials, 0)
n_steps = int(len(vm) / n_trials)    # int to be absolutely Py3-safe
vm.shape = (n_trials, n_steps)
...
vm.shape = (n_trials, t_tot)
vm_reference.shape = (n_trials, t_tot)
vm.shape = (n_trials, int(t_tot))
vm_reference.shape = (n_trials, int(t_tot))

vm_mean = numpy.array([numpy.mean(vm[:, i])
for i in range(int(t_tot))])

This comment has been minimized.

@heplesser

heplesser Mar 16, 2017 Contributor

I think you can avoid the list comprehension here, NumPy supports row- and column-wise means.

Maximilian Schmidt added 2 commits Mar 15, 2017
Convert float variables to integers when using them as indices to numpy arrays and make reshaping of voltmeter more general
@mschmidt87 mschmidt87 force-pushed the mschmidt87:fix_quantal_stdp_test_for_numpy branch from 84b1252 to c4dfd24 Mar 16, 2017
@mschmidt87
Copy link
Author

mschmidt87 commented Mar 16, 2017

Thank you for your feedback. I removed the commit touching .travis.yml from the PR and addressed your comments. I furthermore removed t_plot from the script because it was only used for computing the error and I didn't see a reason to not take the entire dataset into account here.

Copy link
Contributor

hakonsbm left a comment

Looks good to me.

@heplesser heplesser merged commit ad93653 into nest:master Mar 30, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mschmidt87 mschmidt87 deleted the mschmidt87:fix_quantal_stdp_test_for_numpy branch Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.