Pynest microcircuit #451

Merged
merged 16 commits into from Dec 14, 2016

Conversation

@hendrikrth
Contributor

hendrikrth commented Aug 5, 2016

The example files of NEST are lacking a PyNEST version of the microcircuit #450. To compensate this I would like you to include my branch. As reviewers I suggest @jhnnsnk and @mschmidt87.

+May 2016
+
+## Description ##
+This is a PyNEST implementation of the SLI cortical microcircuit from the microcircuit model by Potjans and Diesmann (2014): The cell-type specific

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

This is a PyNEST implementation of the microcircuit model ...
(no need to mention SLI)

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

This is a PyNEST implementation of the microcircuit model ...
(no need to mention SLI)

+
+* Files:
+ * network.py
+ file which gathers all parameters and connects the different nodes with each other

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

remove 'file which' to be consistent

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

remove 'file which' to be consistent

+ contains the parameters for the stimuli
+ * example.py
+ use this script to try out the microcircuit
+

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

  • Please put a '.' behind each sentence.
  • Please start the lase sentence with a capital letter: "Use this script..."
@mschmidt87

mschmidt87 Sep 9, 2016

  • Please put a '.' behind each sentence.
  • Please start the lase sentence with a capital letter: "Use this script..."
+
+How to use the example:
+
+To run the microcircuit on a local machine, adjust the variables N\_scaling and K\_scaling in network_params.py to 0.1 and create an output directory called '/data'. Then run 'example.py' with python. The output will be saved in the directory '/data'.

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor
  • give an explanation what N_scaling and K_scaling mean and state that the full network refers to the value 1; mention that 0.1 is the default here
  • the output directory could be generated automatically
  • for running, use 'python example.py'
@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor
  • give an explanation what N_scaling and K_scaling mean and state that the full network refers to the value 1; mention that 0.1 is the default here
  • the output directory could be generated automatically
  • for running, use 'python example.py'
+
+To run the microcircuit on a local machine, adjust the variables N\_scaling and K\_scaling in network_params.py to 0.1 and create an output directory called '/data'. Then run 'example.py' with python. The output will be saved in the directory '/data'.
+The code can be parallelized using OpenMP and MPI, if NEST has been build with these applications. The number of threads (per MPI process) can be chosen by adjusting local\_num\_threads in sim_params.py. The number of MPI process can be set by running the script with the command 'mpirun -n num\_MPI\_processes python example.py'.
+

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

Please put the variable names and code examples in code blocks to improve the formatting, like so: N_scaling.

@mschmidt87

mschmidt87 Sep 9, 2016

Please put the variable names and code examples in code blocks to improve the formatting, like so: N_scaling.

+How to use the example:
+
+To run the microcircuit on a local machine, adjust the variables N\_scaling and K\_scaling in network_params.py to 0.1 and create an output directory called '/data'. Then run 'example.py' with python. The output will be saved in the directory '/data'.
+The code can be parallelized using OpenMP and MPI, if NEST has been build with these applications. The number of threads (per MPI process) can be chosen by adjusting local\_num\_threads in sim_params.py. The number of MPI process can be set by running the script with the command 'mpirun -n num\_MPI\_processes python example.py'.

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

@mschmidt87

mschmidt87 Sep 9, 2016

+The code can be parallelized using OpenMP and MPI, if NEST has been build with these applications. The number of threads (per MPI process) can be chosen by adjusting local\_num\_threads in sim_params.py. The number of MPI process can be set by running the script with the command 'mpirun -n num\_MPI\_processes python example.py'.
+
+Tested configuration:
+This version has been tested with NEST 2.10.0, Python 2.7.12, NumPy 1.11.1

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

Please insert the missing '.' at the end.

@mschmidt87

mschmidt87 Sep 9, 2016

Please insert the missing '.' at the end.

+Hendrik Rothe, Hannah Bos, Sacha van Albada; May 2016
+'''
+
+from network_params import *

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

to avoid import *, use for example from network_params import net_dict (the other import accordingly)

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

to avoid import *, use for example from network_params import net_dict (the other import accordingly)

+from sim_params import *
+from stimulus_params import *
+import time
+import network as network

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

import network is enough

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

import network is enough

+# connect all nodes
+t1 = time.time()
+net.connect()
+print 'time to create the connections', time.time()-t1

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor
  • specify seconds as unit
  • format the number, e.g. 2 decimals
  • 'connections:'
  • for consistency, start all printouts with capital letters
@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor
  • specify seconds as unit
  • format the number, e.g. 2 decimals
  • 'connections:'
  • for consistency, start all printouts with capital letters
+# along with NEST. If not, see <http://www.gnu.org/licenses/>.
+
+'''
+pynest microcicuit simulation parameters

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

--> microcircuit

@mschmidt87

mschmidt87 Sep 9, 2016

--> microcircuit

+sim_dict = {
+ # Simulation time (in ms)
+ 't_sim': 1000.0,
+ # sim_resolution = Resolution of the simulation (in ms)

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

Please make the explanations consistent, so remove the 'sim_resolution = ' here.

@mschmidt87

mschmidt87 Sep 9, 2016

Please make the explanations consistent, so remove the 'sim_resolution = ' here.

+
+import os
+current_dir = os.getcwd()
+full_path = current_dir + '/data/'

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

  • Please use os.path.join to concatenate the paths.
  • Here, you can place a command to create the actual data path if it does not exist.
@mschmidt87

mschmidt87 Sep 9, 2016

  • Please use os.path.join to concatenate the paths.
  • Here, you can place a command to create the actual data path if it does not exist.
+ 'master_seed': 55,
+ # Number of threads run by NEST
+ 'local_num_threads': 1,
+ # Recording intervall of the membrane potential (in ms)

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

--> interval

+ 'local_num_threads': 1,
+ # Recording intervall of the membrane potential (in ms)
+ 'rec_V_int': 1.0,
+ # If True data will be overwritten, if False it won't

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

should be: "If True, data will be overwritten. If False, a NESTError is raised if the files already exist."

@mschmidt87

mschmidt87 Sep 9, 2016

should be: "If True, data will be overwritten. If False, a NESTError is raised if the files already exist."

+ 'data_path': full_path,
+ # Masterseed for NEST and NumPy
+ 'master_seed': 55,
+ # Number of threads run by NEST

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

--> "Number of threads per MPI process"

@mschmidt87

mschmidt87 Sep 9, 2016

--> "Number of threads per MPI process"

+def get_std_delays(std_delay_exc, std_delay_inh, number_of_pop):
+ """Creates matrix containing the standard deviations of all delays.
+
+ Arguments

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

Arguments:

+ evoked postsynaptic potential.
+
+ Arguements:
+ PSP_rel: standard deviation of the evoked synaptiv potential

This comment has been minimized.

+ inh = PSP_e * g
+ weights[:, 0:dim:2] = exc
+ weights[:, 1:dim:2] = inh
+ # The weight of the connection L23E to L4E is doubled.

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

This comment should be contained in the docsctrings of the function.
And the connection from 4E onto 23E is doubled.

@mschmidt87

mschmidt87 Sep 9, 2016

This comment should be contained in the docsctrings of the function.
And the connection from 4E onto 23E is doubled.

+
+
+def get_mean_delays(mean_delay_exc, mean_delay_inh, number_of_pop):
+ """Creates matrix containing the delay of all connections.

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

docstring conventions, e.g., as used by topology (compare numpy); be also consistent with the other files

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

docstring conventions, e.g., as used by topology (compare numpy); be also consistent with the other files

+ std_value_normal = PSP_rel
+ std_value_exception = 0.05
+ std_mat[:, :] = std_value_normal
+ std_mat[0, 2] = std_value_exception

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

What is the reason for setting the relative standard deviation here different than in the other connections? We find this neither in the SLI implementation nor in the paper.

@mschmidt87

mschmidt87 Sep 9, 2016

What is the reason for setting the relative standard deviation here different than in the other connections? We find this neither in the SLI implementation nor in the paper.

+ """This function creates a matrix of the standard deviation of the
+ evoked postsynaptic potential.
+
+ Arguements:

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

Arguments:

+ # The default recording device is the spike_detector. If you also
+ # want to record the membrane potentials of the neurons, add
+ # 'voltmeter' to the list.
+ 'rec_dev': ['spike_detector', 'voltmeter'],

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

either remove 'voltmeter' from the list or adapt the documentation

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

either remove 'voltmeter' from the list or adapt the documentation

+ 'N_scaling': 0.1,
+ # Amplitude of excitatory postsynaptic potential.
+ 'PSP_e': 0.15,
+ # Standard deviation of the postsynaptic potential.

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

Should be "Relative standard deviation.."

@mschmidt87

mschmidt87 Sep 9, 2016

Should be "Relative standard deviation.."

+ 'mean_delay_matrix': get_mean_delays(1.5, 0.75, 8),
+ # std delay matrix.
+ 'std_delay_matrix': get_std_delays(0.75, 0.375, 8),
+ # Rate of the poissonian spike generator.

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

--> "Poissonian"

@mschmidt87

mschmidt87 Sep 9, 2016

--> "Poissonian"

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

Define all values explicitly in the net_dict (e.g. g is missing).
I suggest to take out the derived matrices from the dictionary, compute them afterwards and update the dictionary with the calculated dependent parameters.
Avoid to define the same variable twice like PSP_e and the first argument of PSP_mean_matrix().

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

Define all values explicitly in the net_dict (e.g. g is missing).
I suggest to take out the derived matrices from the dictionary, compute them afterwards and update the dictionary with the calculated dependent parameters.
Avoid to define the same variable twice like PSP_e and the first argument of PSP_mean_matrix().

+ 'bg_rate': 8.,
+ # Turn Poisson input on or off (True or False).
+ 'poisson_input': True,
+ # Delay of the poisson generator

This comment has been minimized.

+ 'poisson_input': True,
+ # Delay of the poisson generator
+ 'poisson_delay': 1.5,
+ # Parameter of the neurons.

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

Parameters

+ 'E_L': -65.0,
+ # Threshold potential of the neurons (in mV).
+ 'V_th': -50.0,
+ # Membrane potential after a spike (in mV).

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

Reset membrane potential...

@mschmidt87

mschmidt87 Sep 9, 2016

Reset membrane potential...

+ 'tau_m': 10.0,
+ # Time constant of postsynaptic excitatory currents (in ms).
+ 'tau_ex': 0.5,
+ # Time constant of postsynaptic inhibitatory currents (in ms).

This comment has been minimized.

@mschmidt87

mschmidt87 Sep 9, 2016

inhibitory

+ # Factor to scale the number of neurons.
+ 'N_scaling': 0.1,
+ # Amplitude of excitatory postsynaptic potential.
+ 'PSP_e': 0.15,

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

unit missing

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

unit missing

+ # std delay matrix.
+ 'std_delay_matrix': get_std_delays(0.75, 0.375, 8),
+ # Rate of the poissonian spike generator.
+ 'bg_rate': 8.,

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

unit missing

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

unit missing

+ # Turn Poisson input on or off (True or False).
+ 'poisson_input': True,
+ # Delay of the poisson generator
+ 'poisson_delay': 1.5,

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

unit missing

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

unit missing

+ # Duration of the thalamic input (in ms)
+ 'th_duration': 10.0,
+ # Rate of the thalamic input
+ 'th_rate': 80.0,

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

unit missing

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

unit missing

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor
  • The thalamic rate in the SLI microcircuit is 120 1/s to reproduce the figure in the paper (there, however, a rate of 15 is mentioned). Please clarify why 80 is used here.
  • The default values for the time interval of thalamic input could be the same for the SLI and the PyNEST version.
@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor
  • The thalamic rate in the SLI microcircuit is 120 1/s to reproduce the figure in the paper (there, however, a rate of 15 is mentioned). Please clarify why 80 is used here.
  • The default values for the time interval of thalamic input could be the same for the SLI and the PyNEST version.
+ # Delay of the thalamic input (in ms)
+ 'delay_th': np.asarray([1.5 for i in range(8)]),
+ # Standard deviation of the thalamic delay
+ 'delay_th_sd': np.asarray([0.75 for i in range(8)]),

This comment has been minimized.

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

unit missing

@jhnnsnk

jhnnsnk Sep 9, 2016

Contributor

unit missing

@hendrikrth

This comment has been minimized.

Show comment
Hide comment
@hendrikrth

hendrikrth Oct 19, 2016

Contributor

Hi,
I have inherited your comments about the mistake in the comment (...weight of L23E to L4E...) and changed it. I removed the exception for the standard deviation, so that all standard deviations are 10 % now (-->1).
I also inherited your feedback about the initialization of the membrane potentials (-->3).
Adressing points 2 and 6:
I still believe that creating the dir and calculating the firing rates, creating the raster and box plot only if nest.Rank() == 0 is not a save way to do. My concerns come from the fact, that the MPI process creating the dir might be slower than the others. If this happens the other MPI process might try to store files in a dir, which has not been created yet. My second concern is that if only one nest.Rank() == 0 plots the results and the others haven't finished yet, there might be some lacking information.
Therefore I propose, that I write a bash script in which the user defines if the simulation should be run with MPI process and defines also the number of them. This script then creates the output directory and executes after finishing the simulation either with MPI or without an analyze script. What are your thoughts about this?

Contributor

hendrikrth commented Oct 19, 2016

Hi,
I have inherited your comments about the mistake in the comment (...weight of L23E to L4E...) and changed it. I removed the exception for the standard deviation, so that all standard deviations are 10 % now (-->1).
I also inherited your feedback about the initialization of the membrane potentials (-->3).
Adressing points 2 and 6:
I still believe that creating the dir and calculating the firing rates, creating the raster and box plot only if nest.Rank() == 0 is not a save way to do. My concerns come from the fact, that the MPI process creating the dir might be slower than the others. If this happens the other MPI process might try to store files in a dir, which has not been created yet. My second concern is that if only one nest.Rank() == 0 plots the results and the others haven't finished yet, there might be some lacking information.
Therefore I propose, that I write a bash script in which the user defines if the simulation should be run with MPI process and defines also the number of them. This script then creates the output directory and executes after finishing the simulation either with MPI or without an analyze script. What are your thoughts about this?

@mschmidt87

This comment has been minimized.

Show comment
Hide comment
@mschmidt87

mschmidt87 Oct 20, 2016

Thanks for addressing our points.

I still believe that creating the dir and calculating the firing rates, creating the raster and box plot only if nest.Rank() == 0 is not a save way to do. My concerns come from the fact, that the MPI process creating the dir might be slower than the others. If this happens the other MPI process might try to store files in a dir, which has not been created yet. My second concern is that if only one nest.Rank() == 0 plots the results and the others haven't finished yet, there might be some lacking information.

The MPI ranks are synchronized in NEST at the beginning and the end of the simulation. So, this should not be a problem.

Thanks for addressing our points.

I still believe that creating the dir and calculating the firing rates, creating the raster and box plot only if nest.Rank() == 0 is not a save way to do. My concerns come from the fact, that the MPI process creating the dir might be slower than the others. If this happens the other MPI process might try to store files in a dir, which has not been created yet. My second concern is that if only one nest.Rank() == 0 plots the results and the others haven't finished yet, there might be some lacking information.

The MPI ranks are synchronized in NEST at the beginning and the end of the simulation. So, this should not be a problem.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Oct 28, 2016

Contributor

@mschmidt87 @jhnnsnk From your most recent comments, I am not quite sure whether you approve the PR as it now is or whether you still require changes. Could you use the "Review Changes" button in the "Files changed" tab and check off whether you approve or require changes?

Contributor

heplesser commented Oct 28, 2016

@mschmidt87 @jhnnsnk From your most recent comments, I am not quite sure whether you approve the PR as it now is or whether you still require changes. Could you use the "Review Changes" button in the "Files changed" tab and check off whether you approve or require changes?

@mschmidt87

Very nice work again. I just have some minor requests regarding comments and the question about the sentence in the readme file.

pynest/examples/Potjans_2014/readme.md
+How to use the example:
+
+To run the microcircuit on a local machine, adjust the variables `N_scaling` and `K_scaling` in `network_params.py` to 0.1 and create an output directory called `data`. `N_scaling` adjusts the number of neurons and `K_scaling` the number of connections to be simulated. The full network can be run by adjusting these values to 1. If this is done, the option to print the time progress should be set to False in the file `sim_params.py`. For running, use `python example.py`. The output will be saved in the directory `data`.
+The code can be parallelized using OpenMP and MPI, if NEST has been built with these applications [Parralel computing with NEST](http://www.nest-simulator.org/parallel_computing/). The number of threads (per MPI process) can be chosen by adjusting `local_num_threads` in `sim_params.py`. The number of MPI process can be set by choosing a reasonable value for `num_mpi_prc` and then running the script with the command `mpirun -n num_mpi_prc` `python` `example.py`. If the microcircuit is not run on one MPI process and on one thread, it is not possible to results of the simulation afterwards.

This comment has been minimized.

@mschmidt87

mschmidt87 Oct 28, 2016

  • Please put brackets around the link "Parallel computing with NEST" and fix the typo.
  • Perhaps you can start a new paragraph before "The code can be parallelized..."
  • What do you mean with the last sentence? I can't make sense out of it.
@mschmidt87

mschmidt87 Oct 28, 2016

  • Please put brackets around the link "Parallel computing with NEST" and fix the typo.
  • Perhaps you can start a new paragraph before "The code can be parallelized..."
  • What do you mean with the last sentence? I can't make sense out of it.

This comment has been minimized.

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

I incorporated your comments.
The sentence ' If the microcircuit is not run on one MPI process... ' does not make any sense anymore. I wanted to state that the evaluation is only possible, when the simulation is run on one MPI processes and one thread. But since you told me, that MPI processes are synchronized by NEST at the beginning and end of the simulation I removed this sentence.

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

I incorporated your comments.
The sentence ' If the microcircuit is not run on one MPI process... ' does not make any sense anymore. I wanted to state that the evaluation is only possible, when the simulation is run on one MPI processes and one thread. But since you told me, that MPI processes are synchronized by NEST at the beginning and end of the simulation I removed this sentence.

+ Contains the parameters for the stimuli.
+ * `example.py`
+ Use this script to try out the microcircuit.
+

This comment has been minimized.

@mschmidt87

mschmidt87 Oct 28, 2016

A description of helpers.py is missing here.

@mschmidt87

mschmidt87 Oct 28, 2016

A description of helpers.py is missing here.

This comment has been minimized.

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

I added a description

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

I added a description

+ 'conn_probs_th':
+ np.array([0.0, 0.0, 0.0983, 0.0619, 0.0, 0.0, 0.0512, 0.0196]),
+ # Delay of the thalamic input (in ms)
+ 'delay_th': np.asarray([1.5 for i in list(range(8))]),

This comment has been minimized.

@mschmidt87

mschmidt87 Oct 28, 2016

Please explain in the comment that this is the mean value of the delays.

@mschmidt87

mschmidt87 Oct 28, 2016

Please explain in the comment that this is the mean value of the delays.

This comment has been minimized.

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted

+ 'K_scaling': 0.1,
+ # Factor to scale the number of neurons.
+ 'N_scaling': 0.1,
+ # Amplitude of excitatory postsynaptic potential (in mV).

This comment has been minimized.

@mschmidt87

mschmidt87 Oct 28, 2016

Please explain that this is the mean value of the PSPs.

@mschmidt87

mschmidt87 Oct 28, 2016

Please explain that this is the mean value of the PSPs.

This comment has been minimized.

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted

+ 'dc_input': False,
+ # Number of thalamic neurons
+ 'n_thal': 902,
+ # Amplitude of the thalamic postsynaptic potential (in mV)

This comment has been minimized.

@mschmidt87

mschmidt87 Oct 28, 2016

Please explain that this is the mean value of the PSPs.

@mschmidt87

mschmidt87 Oct 28, 2016

Please explain that this is the mean value of the PSPs.

This comment has been minimized.

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted

pynest/examples/Potjans_2014/network.py
+ )
+
+ def connect_poisson(self):
+ """ Connects the Poisson generator to the microcircuit."""

This comment has been minimized.

@mschmidt87

mschmidt87 Oct 28, 2016

change to "Poisson generators" to avoid the impression that all neurons share one generator.

@mschmidt87

mschmidt87 Oct 28, 2016

change to "Poisson generators" to avoid the impression that all neurons share one generator.

This comment has been minimized.

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted

pynest/examples/Potjans_2014/network.py
+ self.connect_devices()
+
+ def simulate(self):
+ """ This function starts the simulation."""

This comment has been minimized.

@mschmidt87

mschmidt87 Oct 28, 2016

It does not only start the simulation, thus it should read e.g.
""" This function simulates the network."""

@mschmidt87

mschmidt87 Oct 28, 2016

It does not only start the simulation, thus it should read e.g.
""" This function simulates the network."""

This comment has been minimized.

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted and changed

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted and changed

pynest/examples/Potjans_2014/example.py
+from stimulus_params import stim_dict
+
+
+# initialize the network and pass parameters to it

This comment has been minimized.

@mschmidt87

mschmidt87 Oct 28, 2016

To be consistent through all script, please capitalize the first word of each comment here.

@mschmidt87

mschmidt87 Oct 28, 2016

To be consistent through all script, please capitalize the first word of each comment here.

This comment has been minimized.

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted and done

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted and done

pynest/examples/Potjans_2014/example.py
+net.simulate()
+time_sim = time.time()-t2
+print("Time to simulate: %.2f seconds" % time_sim)
+# plot results of the simulation

This comment has been minimized.

@mschmidt87

mschmidt87 Oct 28, 2016

Some more explanation would be nice here, e.g.
" Plot a raster plot of the simulated spikes and the average spike rates of all populations. For visual purposes, the last 200 ms are plotted here by default. To compute the spike rates, the first 500 ms of the simulation are discarded to neglect initial transients."

@mschmidt87

mschmidt87 Oct 28, 2016

Some more explanation would be nice here, e.g.
" Plot a raster plot of the simulated spikes and the average spike rates of all populations. For visual purposes, the last 200 ms are plotted here by default. To compute the spike rates, the first 500 ms of the simulation are discarded to neglect initial transients."

This comment has been minimized.

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted and added further explanation

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

Accepted and added further explanation

pynest/examples/Potjans_2014/helpers.py
+ for h in list(range(len(files))):
+ n_fil = data_all[h][:, 0]
+ n_fil = n_fil.astype(int)
+ t_fil = data_all[h][:, 1]

This comment has been minimized.

@mschmidt87

mschmidt87 Oct 28, 2016

t_fil is introduced but never used.

@mschmidt87

mschmidt87 Oct 28, 2016

t_fil is introduced but never used.

This comment has been minimized.

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

I removed this variable

@hendrikrth

hendrikrth Oct 31, 2016

Contributor

I removed this variable

incorporated comments by mschmidt87
fixed typos, removed unnecessary comments and variables
corrected the readme and added information about helpers.py
+ }
+
+
+stim_dict.update(updated_dict)

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 6, 2016

I don't understand the newly added construction with updated_dict here. In contrast to net_dict in network_params.py, this seems unnecessary here, because you don't use any parameters from stim_dict in updated_dict.

@mschmidt87

mschmidt87 Nov 6, 2016

I don't understand the newly added construction with updated_dict here. In contrast to net_dict in network_params.py, this seems unnecessary here, because you don't use any parameters from stim_dict in updated_dict.

This comment has been minimized.

@hendrikrth

hendrikrth Nov 6, 2016

Contributor

Yes, you are right that made no sense. I removed the update routine.

@hendrikrth

hendrikrth Nov 6, 2016

Contributor

Yes, you are right that made no sense. I removed the update routine.

@mschmidt87

This comment has been minimized.

Show comment
Hide comment
@mschmidt87

mschmidt87 Nov 6, 2016

👍 from me.

👍 from me.

pynest/examples/Potjans_2014/network.py
+ 'Seeds for random number generators of virtual processes: %r'
+ % rng_seeds
+ )
+ print('Global random number generator seed:%i' % grng_seed)

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

Add a space in front of %i for being consistent.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

Add a space in front of %i for being consistent.

This comment has been minimized.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

pynest/examples/Potjans_2014/network.py
+
+ def connect_devices(self):
+ """ Connects the recording devices to the microcircuit."""
+ print('%s of 2 Devices connected' % (len(self.net_dict['rec_dev'])))

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

It prints "1 of 2 Devices connected" if only a spike detector is used which sounds as if something was missing. I suggest to be specific and print "Spike Detector connected" or/and "Voltmeter connected".

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

It prints "1 of 2 Devices connected" if only a spike detector is used which sounds as if something was missing. I suggest to be specific and print "Spike Detector connected" or/and "Voltmeter connected".

This comment has been minimized.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

fixed

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

fixed

pynest/examples/Potjans_2014/example.py
+
+
+# Initialize the network and pass parameters to it.
+net = network.Network(sim_dict, net_dict, stim_dict)

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

It would be nice to time also the network initialization.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

It would be nice to time also the network initialization.

This comment has been minimized.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

pynest/examples/Potjans_2014/example.py
+t1 = time.time()
+net.setup()
+time_con = time.time()-t1
+print("Time to create the connections: %.2f seconds" % time_con)

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

Since the units ms and Hz are printed as well, why not just writing s instead of seconds?

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

Since the units ms and Hz are printed as well, why not just writing s instead of seconds?

This comment has been minimized.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

pynest/examples/Potjans_2014/readme.md
+
+How to use the example:
+
+To run the microcircuit on a local machine, adjust the variables `N_scaling` and `K_scaling` in `network_params.py` to 0.1 and create an output directory called `data`. `N_scaling` adjusts the number of neurons and `K_scaling` the number of connections to be simulated. The full network can be run by adjusting these values to 1. If this is done, the option to print the time progress should be set to False in the file `sim_params.py`. For running, use `python example.py`. The output will be saved in the directory `data`.

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

Remove

and create an output directory called data.

since this is now done automatically.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

Remove

and create an output directory called data.

since this is now done automatically.

This comment has been minimized.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Fixed

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Fixed

pynest/examples/Potjans_2014/network.py
+ """ This function simulates the microcircuit."""
+ nest.Simulate(self.sim_dict['t_sim'])
+
+ def evaluate(self, raster_plot_time_idx, fire_rate_time_idx):

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

You could save the generated plots directly in the output directory together with the raw data.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

You could save the generated plots directly in the output directory together with the raw data.

This comment has been minimized.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

+ # Standard deviation of the postsynaptic potential (in relative units).
+ 'PSP_sd': 0.1,
+ # Start of the thalamic input (in ms).
+ 'th_start': 700.0,

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

It would be nice to set the default stimulus intervals somewhere into the default plotting interval so that their effect is directly visible by eye.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

It would be nice to set the default stimulus intervals somewhere into the default plotting interval so that their effect is directly visible by eye.

This comment has been minimized.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

I kept the thalamus stimulus interval, since this is the stimulus interval of the sli version. I therefore changed the interval of the spike raster plot.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

I kept the thalamus stimulus interval, since this is the stimulus interval of the sli version. I therefore changed the interval of the spike raster plot.

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 21, 2016

Contributor

Ok. Please update the comment on the plotting interval accordingly.

@jhnnsnk

jhnnsnk Nov 21, 2016

Contributor

Ok. Please update the comment on the plotting interval accordingly.

pynest/examples/Potjans_2014/network.py
+ else:
+ os.mkdir(self.sim_dict['data_path'])
+ print('data directory created')
+ print('Data will be written to %s' % self.data_path)

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

If used with MPI such statements are currently printed once per process. Please print them only on Rank 0 unless you want to print something specific to a process.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

If used with MPI such statements are currently printed once per process. Please print them only on Rank 0 unless you want to print something specific to a process.

This comment has been minimized.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

pynest/examples/Potjans_2014/network.py
+ '0 << /model /%s >> GetLocalNodes' % self.net_dict['neuron_model']
+ )
+ local_nodes = nest.sli_pop()
+ vp = nest.GetStatus(local_nodes)[0]['vp']

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

If run with both MPI and threads, taking the vp of the first local node is not correct. For example, with 2 MPI processes and 2 threads, one gets the MPI ranks 0 and 1, but the vps 0,1,2 and 3. For accessing the right pyrngs, I think we cannot avoid to extract all vps correponding to the local nodes. I will give a suggestion soon.

@jhnnsnk

jhnnsnk Nov 8, 2016

Contributor

If run with both MPI and threads, taking the vp of the first local node is not correct. For example, with 2 MPI processes and 2 threads, one gets the MPI ranks 0 and 1, but the vps 0,1,2 and 3. For accessing the right pyrngs, I think we cannot avoid to extract all vps correponding to the local nodes. I will give a suggestion soon.

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 9, 2016

Contributor

This might be a possible solution to the problem:

for thread in np.arange(nest.GetKernelStatus('local_num_threads')):
    local_nodes = nest.GetNodes([0], {'model': self.net_dict['neuron_model'], 'thread': thread}, local_only=True)[0]
    vp = nest.GetStatus(local_nodes)[0]['vp'] # same thread on MPI proc -> same vp
    nest.SetStatus(
        local_nodes, 'V_m', self.pyrngs[vp].normal(
        self.net_dict['neuron_params']['V0_mean'],
        self.net_dict['neuron_params']['V0_sd'],
            len(local_nodes)))
@jhnnsnk

jhnnsnk Nov 9, 2016

Contributor

This might be a possible solution to the problem:

for thread in np.arange(nest.GetKernelStatus('local_num_threads')):
    local_nodes = nest.GetNodes([0], {'model': self.net_dict['neuron_model'], 'thread': thread}, local_only=True)[0]
    vp = nest.GetStatus(local_nodes)[0]['vp'] # same thread on MPI proc -> same vp
    nest.SetStatus(
        local_nodes, 'V_m', self.pyrngs[vp].normal(
        self.net_dict['neuron_params']['V0_mean'],
        self.net_dict['neuron_params']['V0_sd'],
            len(local_nodes)))

This comment has been minimized.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

I fixed this, using your solution.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

I fixed this, using your solution.

pynest/examples/Potjans_2014/network.py
+ self.DC_amp_e = compute_DC(self.net_dict, self.w_ext)
+
+ print(
+ 'The Number of neurons is scaled by a factor of: %.2f'

This comment has been minimized.

@jhnnsnk

jhnnsnk Nov 9, 2016

Contributor

Number -> number

@jhnnsnk

jhnnsnk Nov 9, 2016

Contributor

Number -> number

This comment has been minimized.

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

@hendrikrth

hendrikrth Nov 17, 2016

Contributor

Done

@jhnnsnk

This comment has been minimized.

Show comment
Hide comment
@jhnnsnk

jhnnsnk Nov 9, 2016

Contributor

Hi, during testing with both MPI and threads, I noticed that the initialization of membrane voltages did not work correctly. I made a suggestion inline how this could be solved. Apart from this, I made some other minor comments for improvement.

Contributor

jhnnsnk commented Nov 9, 2016

Hi, during testing with both MPI and threads, I noticed that the initialization of membrane voltages did not work correctly. I made a suggestion inline how this could be solved. Apart from this, I made some other minor comments for improvement.

hendrikrth added some commits Nov 17, 2016

included comments by jhnnsnk
fixed initialization of membrane potentials, with provided solution
@mschmidt87

Hi, thanks to @albada , I took another look at the code and discovered some typos. Please fix them.

pynest/examples/Potjans_2014/example.py
+# along with NEST. If not, see <http://www.gnu.org/licenses/>.
+
+'''
+pynest microcicuit example

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 24, 2016

Please correct to microcircuit. Also in All other files that have this typo.

@mschmidt87

mschmidt87 Nov 24, 2016

Please correct to microcircuit. Also in All other files that have this typo.

This comment has been minimized.

@hendrikrth

hendrikrth Nov 30, 2016

Contributor

fixed this and all other typos

@hendrikrth

hendrikrth Nov 30, 2016

Contributor

fixed this and all other typos

pynest/examples/Potjans_2014/helpers.py
+
+
+def get_weight(PSP_val, net_dict):
+ """ Function computes weight to elicit a change in the membrane potential.

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 24, 2016

Please change to "Computes" in accordance to the documentation of other functions.

@mschmidt87

mschmidt87 Nov 24, 2016

Please change to "Computes" in accordance to the documentation of other functions.

This comment has been minimized.

@hendrikrth

hendrikrth Nov 30, 2016

Contributor

Changes this and adapted this style in all other functions

@hendrikrth

hendrikrth Nov 30, 2016

Contributor

Changes this and adapted this style in all other functions

pynest/examples/Potjans_2014/helpers.py
+ )
+ # If the network is scaled the indegrees are calculated in the same
+ # fashion as in the original version of the circuit, which is
+ # written in sli

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 24, 2016

insert '.' at the end.

@mschmidt87

mschmidt87 Nov 24, 2016

insert '.' at the end.

This comment has been minimized.

@hendrikrth

hendrikrth Nov 30, 2016

Contributor

Done

@hendrikrth

hendrikrth Nov 30, 2016

Contributor

Done

pynest/examples/Potjans_2014/helpers.py
+def adj_w_ext_to_K(K_full, K_scaling, w, w_from_PSP, DC, net_dict, stim_dict):
+ """ Adjustment of weights to scaling is dine in this function.
+
+ With this funtion the recurrent and external weights are adjusted

This comment has been minimized.

This comment has been minimized.

@hendrikrth

hendrikrth Nov 30, 2016

Contributor

Done

@hendrikrth

hendrikrth Nov 30, 2016

Contributor

Done

pynest/examples/Potjans_2014/helpers.py
+
+
+def load_spike_times(path, name, begin, end):
+ """ This function loads spike times of each spike detector.

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 24, 2016

Change to "Loads..."

@mschmidt87

mschmidt87 Nov 24, 2016

Change to "Loads..."

This comment has been minimized.

@hendrikrth

hendrikrth Nov 30, 2016

Contributor

Done

@hendrikrth

hendrikrth Nov 30, 2016

Contributor

Done

+# along with NEST. If not, see <http://www.gnu.org/licenses/>.
+
+'''
+microcicuit simulation parameters

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 24, 2016

microcircuit

+microcicuit simulation parameters
+---------------------------------
+
+Simulation Parameters for the microcircuit.

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 24, 2016

Simulation parameters

@mschmidt87

mschmidt87 Nov 24, 2016

Simulation parameters

+ 'local_num_threads': 1,
+ # Recording interval of the membrane potential (in ms).
+ 'rec_V_int': 1.0,
+ # If True data will be overwritten,

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 24, 2016

If True, data...

@mschmidt87

mschmidt87 Nov 24, 2016

If True, data...

+ # Recording interval of the membrane potential (in ms).
+ 'rec_V_int': 1.0,
+ # If True data will be overwritten,
+ # if False a NESTError is raised if the files already exist.

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 24, 2016

If False, ...

+# along with NEST. If not, see <http://www.gnu.org/licenses/>.
+
+'''
+microcicuit stimulus parameters

This comment has been minimized.

@mschmidt87

mschmidt87 Nov 24, 2016

microcircuit

fixed typos and changed a comment
some typos were fixed and a comment about the initialization
of the membrane potentials was changed
@jhnnsnk

This comment has been minimized.

Show comment
Hide comment
@jhnnsnk

jhnnsnk Dec 14, 2016

Contributor

👍

Contributor

jhnnsnk commented Dec 14, 2016

👍

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Dec 14, 2016

Contributor

@hendrikrth Thank you for porting this important model to PyNEST! @jhnnsnk @mschmidt87 Thank you for your thorough review!

Contributor

heplesser commented Dec 14, 2016

@hendrikrth Thank you for porting this important model to PyNEST! @jhnnsnk @mschmidt87 Thank you for your thorough review!

@heplesser heplesser merged commit 102e338 into nest:master Dec 14, 2016

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