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

Suggestion to improve nest.SetKernelStatus() and nest.GetKernelStatus() documentation #1350

Closed
morales-gregorio opened this issue Nov 27, 2019 · 2 comments · Fixed by #1373
Closed
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: Documentation DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL

Comments

@morales-gregorio
Copy link
Contributor

Hello hello!

@rgutzen and I are working on a NEST simulation code class for Networkunit and stumbled across some problems with the Nest kernel parameters.

We wanted to manage the setting of the kernel parameters in a general way such that Networkunit users can use a standardized simulation parameter dictionary instead of writing all the code themselves. While trying to implement this, I found it difficult to find which are all the Kernel parameters and what is their function in the documentation.

Here I describe the process I went through to try to figure out which keywords are accepted for nest.SetKernelStatus() (Just skip to the end to see the suggestion/conclusion)

So I thought that retrieving them would be a good idea (in pynest):

>>> import nest
>>> from pprint import pprint
>>> nest.ResetKernel()
>>> params = nest.GetKernelStatus()
>>> pprint(params)
{'T_max': 1152921504606846.8,
 'T_min': -1152921504606846.8,
 'adaptive_spike_buffers': True,
 'adaptive_target_buffers': True,
 'buffer_size_secondary_events': 0,
 'buffer_size_spike_data': 2,
 'buffer_size_target_data': 2,
 'data_path': '',
 'data_prefix': '',
 'dict_miss_is_error': True,
 'grng_seed': 0,
 'growth_factor_buffer_spike_data': 1.5,
 'growth_factor_buffer_target_data': 1.5,
 'keep_source_table': True,
 'local_num_threads': 1,
 'local_spike_counter': 0,
 'max_buffer_size_spike_data': 8388608,
 'max_buffer_size_target_data': 16777216,
 'max_delay': 0.1,
 'max_num_syn_models': 512,
 'min_delay': 0.1,
 'ms_per_tic': 0.001,
 'network_size': 1,
 'num_connections': 0,
 'num_processes': 1,
 'off_grid_spiking': False,
 'overwrite_files': False,
 'print_time': False,
 'resolution': 0.1,
 'rng_seeds': (1,),
 'sort_connections_by_source': True,
 'structural_plasticity_synapses': {},
 'structural_plasticity_update_interval': 1000,
 'tics_per_ms': 1000.0,
 'tics_per_step': 100,
 'time': 0.0,
 'time_collocate': 0.0,
 'time_communicate': 0.0,
 'to_do': 0,
 'total_num_virtual_procs': 1,
 'use_wfr': True,
 'wfr_comm_interval': 1.0,
 'wfr_interpolation_order': 3,
 'wfr_max_iterations': 15,
 'wfr_tol': 0.0001}

We found there what seems to be the full set of kernel parameters. I thought we could use this as a template for setting the parameters, but it soon turned out to be a bad idea:

>>> nest.SetKernelStatus(params)

Nov 27 13:46:25 SetStatus [Error]: 
    Variable data_path not set: Directory '' does not exist.

Nov 27 13:46:25 ModelManager::clear_models_ [Info]: 
    Models will be cleared and parameters reset.

Nov 27 13:46:25 Network::create_rngs_ [Info]: 
    Deleting existing random number generators

Nov 27 13:46:25 Network::create_rngs_ [Info]: 
    Creating default RNGs

Nov 27 13:46:25 Network::create_grng_ [Info]: 
    Creating new default global RNG

Nov 27 13:46:25 ModelManager::clear_models_ [Info]: 
    Models will be cleared and parameters reset.

Nov 27 13:46:25 Network::create_rngs_ [Info]: 
    Deleting existing random number generators

Nov 27 13:46:25 Network::create_rngs_ [Info]: 
    Creating default RNGs

Nov 27 13:46:25 Network::create_grng_ [Info]: 
    Creating new default global RNG

Nov 27 13:46:25 SimulationManager::set_status [Info]: 
    tics per ms and resolution changed.

Nov 27 13:46:25 SimulationManager::set_status [Info]: 
    Waveform communication interval changed successfully. 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/morales/anaconda2/envs/newt/lib/python3.8/site-packages/nest/ll_api.py", line 246, in stack_checker_func
    return f(*args, **kwargs)
  File "/home/morales/anaconda2/envs/newt/lib/python3.8/site-packages/nest/lib/hl_api_simulation.py", line 243, in SetKernelStatus
    sr('SetStatus')
  File "/home/morales/anaconda2/envs/newt/lib/python3.8/site-packages/nest/ll_api.py", line 132, in catching_sli_run
    raise exceptionCls(commandname, message)
nest.ll_api.DictError: ('DictError in SetStatus_id: Unused dictionary items:  T_max T_min tics_per_step time_collocate time_communicate to_do', 'DictError', <SLILiteral: SetStatus_id>, ': Unused dictionary items:  T_max T_min tics_per_step time_collocate time_communicate to_do')

As @AlexVanMeegen pointed out, some of this parameters probably depend on each other and therefore cannot be set by the user. As a last check I decided to drop the parameters mentioned in the error message:

>>> [params.pop(item) for item in ['T_max', 'T_min', 'tics_per_step', 'time_collocate', 'time_communicate', 'to_do']]
>>> pprint(params)
{'adaptive_spike_buffers': True,
 'adaptive_target_buffers': True,
 'buffer_size_secondary_events': 0,
 'buffer_size_spike_data': 2,
 'buffer_size_target_data': 2,
 'data_path': '',
 'data_prefix': '',
 'dict_miss_is_error': True,
 'grng_seed': 0,
 'growth_factor_buffer_spike_data': 1.5,
 'growth_factor_buffer_target_data': 1.5,
 'keep_source_table': True,
 'local_num_threads': 1,
 'local_spike_counter': 0,
 'max_buffer_size_spike_data': 8388608,
 'max_buffer_size_target_data': 16777216,
 'max_delay': 0.1,
 'max_num_syn_models': 512,
 'min_delay': 0.1,
 'ms_per_tic': 0.001,
 'network_size': 1,
 'num_connections': 0,
 'num_processes': 1,
 'off_grid_spiking': False,
 'overwrite_files': False,
 'print_time': False,
 'resolution': 0.1,
 'rng_seeds': (1,),
 'sort_connections_by_source': True,
 'structural_plasticity_synapses': {},
 'structural_plasticity_update_interval': 1000,
 'tics_per_ms': 1000.0,
 'time': 0.0,
 'total_num_virtual_procs': 1,
 'use_wfr': True,
 'wfr_comm_interval': 1.0,
 'wfr_interpolation_order': 3,
 'wfr_max_iterations': 15,
 'wfr_tol': 0.0001}

But this also did not really solve the problem, since there seems to still be some dependencies among the parameters.

>>> nest.SetKernelStatus(params)

Nov 27 13:54:35 SetStatus [Error]: 
    Variable data_path not set: Directory '' does not exist.
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/morales/anaconda2/envs/newt/lib/python3.8/site-packages/nest/ll_api.py", line 246, in stack_checker_func
    return f(*args, **kwargs)
  File "/home/morales/anaconda2/envs/newt/lib/python3.8/site-packages/nest/lib/hl_api_simulation.py", line 243, in SetKernelStatus
    sr('SetStatus')
  File "/home/morales/anaconda2/envs/newt/lib/python3.8/site-packages/nest/ll_api.py", line 132, in catching_sli_run
    raise exceptionCls(commandname, message)
nest.ll_api.: ('Delay extrema have been set: Thread/process number cannot be changed. in SetStatus_id: ', 'Delay extrema have been set: Thread/process number cannot be changed.', <SLILiteral: SetStatus_id>, ': ')

I thought that probably this did not work because I had not reset the kernel and indeed:

>>> nest.ResetKernel()

Nov 27 13:55:37 ModelManager::clear_models_ [Info]: 
    Models will be cleared and parameters reset.

Nov 27 13:55:37 Network::create_rngs_ [Info]: 
    Deleting existing random number generators

Nov 27 13:55:37 Network::create_rngs_ [Info]: 
    Creating default RNGs

Nov 27 13:55:37 Network::create_grng_ [Info]: 
    Creating new default global RNG
>>> nest.SetKernelStatus(params)

Nov 27 13:55:39 SetStatus [Error]: 
    Variable data_path not set: Directory '' does not exist.

Nov 27 13:55:39 ModelManager::clear_models_ [Info]: 
    Models will be cleared and parameters reset.

Nov 27 13:55:39 Network::create_rngs_ [Info]: 
    Deleting existing random number generators

Nov 27 13:55:39 Network::create_rngs_ [Info]: 
    Creating default RNGs

Nov 27 13:55:39 Network::create_grng_ [Info]: 
    Creating new default global RNG

Nov 27 13:55:39 ModelManager::clear_models_ [Info]: 
    Models will be cleared and parameters reset.

Nov 27 13:55:39 Network::create_rngs_ [Info]: 
    Deleting existing random number generators

Nov 27 13:55:39 Network::create_rngs_ [Info]: 
    Creating default RNGs

Nov 27 13:55:39 Network::create_grng_ [Info]: 
    Creating new default global RNG

Nov 27 13:55:39 SimulationManager::set_status [Info]: 
    tics per ms and resolution changed.

Nov 27 13:55:39 SimulationManager::set_status [Info]: 
    Waveform communication interval changed successfully. 

All in all, I am still not sure if ['T_max', 'T_min', 'tics_per_step', 'time_collocate', 'time_communicate', 'to_do'] are the only parameters that depend on the others, or if there is another reason for them to raise an error when trying to set them.

Conclusion

Right now the documentation of nest.SetKernelStatus() and nest.GetKernelStatus() are unfortunately not very informative.

>>> print(nest.SetKernelStatus.__doc__)
Set parameters for the simulation kernel.

    Parameters
    ----------
    params : dict
        Dictionary of parameters to set.

    See Also
    --------
    GetKernelStatus

    KEYWORDS:

>>> print(nest.GetKernelStatus.__doc__)
Obtain parameters of the simulation kernel.

    Parameters
    ----------
    keys : str or list, optional
        Single parameter name or ``list`` of parameter names

    Returns
    -------
    dict:
        Parameter dictionary, if called without argument
    type:
        Single parameter value, if called with single parameter name
    list:
        List of parameter values, if called with list of parameter names

    Raises
    ------
    TypeError
        If `keys` are of the wrong type.

    See Also
    --------
    SetKernelStatus

    KEYWORDS:
    

I think that a list of the possible keys and a very brief description of what they are (in the docstring) would help the users a lot (specially the newer ones like myself). It would also help if the types and inter-dependencies between these parameters are also stated in the description.

Thanks for reading!

morales-gregorio added a commit to morales-gregorio/NetworkUnit that referenced this issue Nov 28, 2019
…re inter-dependent. Add a check that none of the 'illegal' parameters is set. Standardization of this should wait until NEST issue [1350](nest/nest-simulator#1350)
@jougs
Copy link
Contributor

jougs commented Dec 11, 2019

The first part of this issue is basically a duplicate of #248. The cause of the problem is not so much dependencies between parameters, but rather that some of the parameters in the kernel/node/connection status dictionaries are read-only and using them in a call to Set{Kernel}Status will rightfully fail.

The proper solution to the problem would be to tag each entry as read-only or read-write, so they could easily be filtered out. However, this is pretty hard:

  • Status dictionaries are not just provided by the kernel/node/connection classes, but each object on the way through the call tree of a Get/Set{Kernel}Status call might add things to them or read from them. This means that it is impossible to judge the writability of a certain property at any given location in the code.
  • Even if a more holistic view were available, dictionaries are simple associative arrays that don't have a field for the writability of entries and solving this would require a whole new data structure -- both inside the kernel and in the user interface layers.

There are two basic workarounds:

  • Maintain a list of read-only properties and use it to remove items from your dictionaries before using them in Set{Kernel}Status calls (recommended).
  • Disable dictionary checks when setting dictionaries that contain properties unknown to the kernel: SetKernelStatus({'dict_miss_is_error': False})

Now to the second part: the kernel properties are documented. It's just that the documentation is pretty hidden:

nest.help("kernel")

I'm happily reviewing a pull request adding this information to the docstrings you mentioned!

@morales-gregorio
Copy link
Contributor Author

On it!

@jougs jougs added ZC: Documentation 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 Jan 27, 2020
morales-gregorio added a commit to morales-gregorio/nest-simulator that referenced this issue Apr 2, 2020
…rs. Incuding all parameters discussed in the PR. Closes nest#1350
@terhorstd terhorstd linked a pull request Apr 7, 2020 that will close this issue
@terhorstd terhorstd linked a pull request Apr 7, 2020 that will close this issue
morales-gregorio added a commit to morales-gregorio/nest-simulator that referenced this issue Apr 23, 2020
…rs. Incuding all parameters discussed in the PR. Closes nest#1350
alberto-antonietti added a commit to alberto-antonietti/nest-simulator that referenced this issue Apr 24, 2020
* Added test for NodeCollection array indexing

* Added support for NodeCollection array indexing

* Refactored NodeCollection array indexing

* Added NodeCollection bool array indexing

* Added more checks and error handling to NodeCollection array indexing

* More tests of NodeCollection array indexing

* Formatting cleanup

* Some minor adjustments to take_array_index

* Added test of non-unique nodes when creating a new NodeCollection

* Correctly raise error when creating NodeCollection with non-unique IDs

* Corrected and added checks of array indexing argument

* Reverted change of NodeCollection creation

It is moved to a separate branch.

* added handling of out-of-range slicing values and indexes of NodeCollections

* updated tests for NodeCollection indexing and slicing

* updated test_NodeCollection

* remove comments

* Update documentation for GetKernelStatus and SetKernelStatus parameters. Incuding all parameters discussed in the PR. Closes nest#1350

* remove parameter list from GetKernelStatus in favor of a full list in SetKernelStatus

* fix typo spotted by @sarakonradi

* Add all keys to SetKernelStatus, change notes according to the new documentation structure, remove (read/write) flags, capitalize descriptions, rewrite double --> float

* move (read only) flag next to the type, may improve readability

* Reorganize key values into different sections

* make local_num_threads writeable

* indicate that off_grid_spiking is writeable

* indicate that num_processes is read only

* correct spelling

* should not -> cannot

* internally calculated -> set by the NEST kernel

* Remove notes and update description under `other parameters` heading

* fix possible typo

* Change header and subheader format

* Other parameters -> Params dictionary

* handling of out-of-range start and stop as list/NumPy array slicing

* added testing of out-of-range start and stop values for slicing

* replace Python 3.6 f-strings with old-fashioned string formatting for Python 3.5 compatibility

Co-authored-by: Håkon Mørk <hakon.mork@nmbu.no>
Co-authored-by: gtrensch <g.trensch@fz-juelich.de>
Co-authored-by: Aitor <a.morales-gregorio@fz-juelich.de>
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
Co-authored-by: Jochen Martin Eppler <j.eppler@fz-juelich.de>
Co-authored-by: C. Linssen <c.linssen@fz-juelich.de>
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: Documentation DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants