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

Add option to generate connections from NumPy arrays #1429

Merged
merged 19 commits into from
Apr 20, 2020

Conversation

hakonsbm
Copy link
Contributor

@hakonsbm hakonsbm commented Feb 13, 2020

Connections are created one-to-one based on NumPy arrays of source and target node IDs, and a synapse specification containing NumPy arrays of weights and delays. All arrays have to be of equal length. For efficiency, the PyNEST kernel bypasses SLI and passes a pointer for each array to the C++ level.

It can be used as in the following example:

nest.Create('iaf_psc_alpha', 10)
# sources and targets must be NumPy arrays of equal lengths, with integer dtype
sources = np.array([1, 5, 7], dtype=np.uint64)
targets = np.array([2, 4, 6], dtype=np.uint64)
# weights, delays, and receptor type must be NumPy arrays with lengths equal to 
# that of sources and targets, or unspecified
weights = np.ones(len(sources))
delays = np.ones(len(sources))

# conn_spec cannot be specified
nest.Connect(sources, targets, syn_spec={'weight': weights, 'delay': delays,
                                         'synapse_model': 'static_synapse'})

This replaces the implementation of Connect_nonunique.

Fixes #1423.

@alberto-antonietti
Copy link
Contributor

Hi @hakonsbm!
Does this work also with non-unique sources and/or targets? Something like:

sources = np.array([1, 5, 7, 5], dtype=np.uint64)
targets = np.array([2, 4, 6, 6], dtype=np.uint64)

@hakonsbm
Copy link
Contributor Author

@alberto-antonietti Yes, it is possible to use non-unique elements.

@alberto-antonietti
Copy link
Contributor

alberto-antonietti commented Feb 17, 2020

@alberto-antonietti Yes, it is possible to use non-unique elements.

Great! That is what we would like to have with #1423 and HBP Ticket #482997.

@terhorstd terhorstd added ZC: PyNEST DO NOT USE THIS LABEL I: External API Developers of extensions or other language bindings may need to adapt their code ZP: PR Created DO NOT USE THIS LABEL S: High Should be handled next T: Enhancement New functionality, model or documentation labels Feb 19, 2020
@jougs
Copy link
Contributor

jougs commented Feb 25, 2020

@alberto-antonietti: the reference to this PR in your comment is most likely not what you wanted. Can you please check and update? Thanks!

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.

Very, very nice! My comments are plenty but rather minor. Many thanks!

doc/guides/from_nest2_to_nest3.rst Outdated Show resolved Hide resolved
doc/guides/from_nest2_to_nest3.rst Outdated Show resolved Hide resolved
doc/guides/from_nest2_to_nest3.rst Outdated Show resolved Hide resolved
doc/guides/from_nest2_to_nest3.rst Outdated Show resolved Hide resolved
doc/guides/from_nest2_to_nest3.rst Outdated Show resolved Hide resolved
pynest/nest/lib/hl_api_connections.py Outdated Show resolved Hide resolved
pynest/nest/lib/hl_api_connections.py Outdated Show resolved Hide resolved
pynest/nest/lib/hl_api_connections.py Outdated Show resolved Hide resolved
pynest/nest/lib/hl_api_connections.py Outdated Show resolved Hide resolved
pynest/nest/tests/test_connect_arrays.py Outdated Show resolved Hide resolved
Copy link
Contributor

@sarakonradi sarakonradi left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work! I agree with Jochen's comments. Also added a few minor suggestions!

doc/guides/from_nest2_to_nest3.rst Outdated Show resolved Hide resolved
doc/guides/from_nest2_to_nest3.rst Outdated Show resolved Hide resolved
doc/guides/from_nest2_to_nest3.rst Outdated Show resolved Hide resolved
@@ -194,6 +194,14 @@ class NodeCollection
*/
static NodeCollectionPTR create( const TokenArray& node_ids );

/**
* Create a NodeCollection from a single node ID. Results in a primitive.
Copy link
Contributor

Choose a reason for hiding this comment

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

Results in a primitive: Is this sentence complete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A primitive is a NodeCollection that is contiguous and homogeneous. It is used as a noun throughout the file, so if the wording is confusing, it probably has to be changed everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. In that case it is best to leave primitive as it is.

@alberto-antonietti
Copy link
Contributor

@alberto-antonietti: the reference to this PR in your comment is most likely not what you wanted. Can you please check and update? Thanks!

Hi @jougs, I updated the reference, sorry!

@hakonsbm I do not know if I can add something meaningful to the two reviews of @jougs and @sarakonradi, but I will try to test it as soon as I have a bit of time (given the incoming end of SGA2)

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 addressing all my comments in such a timely fashion. Good to go from my side.

Copy link
Contributor

@alberto-antonietti alberto-antonietti left a comment

Choose a reason for hiding this comment

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

Thanks @hakonsbm for the great work!

I have added a few comments from the "user" perspective, to provide better information about the exact errors that could be triggered.

Do you think that the limitation of setting only synapse_model, weights, delays, and receptors could be relaxed?
I do not know how hard that could be, but I can think of some of my simulations where I connect non-unique neurons (therefore I cannot use the standard nest.Connect() between NodeCollections) and I set other syn_spec, especially if I have synaptic plasticity.

Comment on lines 357 to 361
nest.Create('iaf_psc_alpha', 10)
# Node IDs in the arrays must address existing nodes, but may occur multiple times.
sources = np.array([1, 5, 7, 5], dtype=np.uint64)
targets = np.array([2, 2, 4, 4], dtype=np.uint64)
nest.Connect(sources, targets)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I run this code snippet, I get an error:

----> 5 nest.Connect(sources, targets)

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/ll_api.py in stack_checker_func(*args, **kwargs)
    245     def stack_checker_func(*args, **kwargs):
    246         if not get_debug():
--> 247             return f(*args, **kwargs)
    248         else:
    249             sr('count')

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/lib/hl_api_connections.py in Connect(pre, post, conn_spec, syn_spec, return_synapsecollection)
    224         if return_synapsecollection:
    225             raise ValueError("SynapseCollection cannot be returned when connecting two arrays of node IDs")
--> 226         weights = numpy.array(processed_syn_spec['weight']) if 'weight' in processed_syn_spec else None
    227         delays = numpy.array(processed_syn_spec['delay']) if 'delay' in processed_syn_spec else None
    228         receptor_type = (numpy.array(processed_syn_spec['receptor_type'])

TypeError: argument of type 'NoneType' is not iterable

I do not get it if I specify the synapse model ( nest.Connect(sources, targets, syn_spec={"synapse_model" : "static_synapse"}) )

Would it be useful to force the specification of synapse_model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we are forcing the specification of synapse_model. I just forgot to update the documentation, but it's fixed now.

if len(set(processed_syn_spec.keys()) - set(['weight', 'delay', 'synapse_model', 'receptor_type'])) != 0:
raise ValueError("When connecting two arrays of node IDs, the synapse specification dictionary can "
"only contain weights, delays, synapse model, and r_port.")
connect_arrays(pre, post, weights, delays, receptor_type, synapse_model)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that its documented, however if one calls by mistake something like nest.Connect(pre, post, syn_spec={"synapse_model" : "static_synapse", "weight": 2.0}), the error message he gets is not very informative.

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/ll_api.py in stack_checker_func(*args, **kwargs)
    245     def stack_checker_func(*args, **kwargs):
    246         if not get_debug():
--> 247             return f(*args, **kwargs)
    248         else:
    249             sr('count')

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/lib/hl_api_connections.py in Connect(pre, post, conn_spec, syn_spec, return_synapsecollection)
    237             raise ValueError("When connecting two arrays of node IDs, the synapse specification dictionary can "
    238                              "only contain weights, delays, synapse model, and r_port.")
--> 239         connect_arrays(pre, post, weights, delays, receptor_type, synapse_model)
    240         return
    241 

pynestkernel.pyx in pynestkernel.NESTEngine.connect_arrays()

TypeError: len() of unsized object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently one can create a 0-dimension NumPy array with for example np.array(2.0), which we didn't check for when connecting. I have added some checks for it now.

@@ -233,9 +232,3 @@ def fixdict(d):
projections = fixdict(projections)
Copy link
Contributor

Choose a reason for hiding this comment

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

if "weight" is a list/np.array of len != len(sources) and len(targets), the following error is Raised.
e.g.

n=10
nest.Create("iaf_cond_exp")
pre = np.random.randint(1, n+1, 100)
post = np.copy(pre)
np.random.shuffle(post)
nest.Connect(pre, post, syn_spec={"synapse_model" : "static_synapse", "weight": [2.0, 3.0]})

Error:

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/lib/hl_api_connections.py in Connect(pre, post, conn_spec, syn_spec, return_synapsecollection)
    217     # to the right formats.
    218     processed_syn_spec = _process_syn_spec(
--> 219         syn_spec, processed_conn_spec, len(pre), len(post))
    220 
    221     # If pre and post are arrays of node IDs, and conn_spec is unspecified,

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/lib/hl_api_connection_helpers.py in _process_syn_spec(syn_spec, conn_spec, prelength, postlength)
     74                             raise kernel.NESTError(
     75                                 "'" + key + "' has to be an array of "
---> 76                                 "dimension " + str(prelength) + ", a "
     77                                 "scalar or a dictionary.")
     78                         else:

NESTError: 'weight' has to be an array of dimension 100, a scalar or a dictionary.

If we are connective np.arrays, the error should say that 'weight' has to be an array of dimension 100, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I've updated the error messages.

@Silmathoron
Copy link
Member

Is there a reason against just converting lists or numpy arrays to NodeCollections if such inputs are provided?

@heplesser
Copy link
Contributor

Is there a reason against just converting lists or numpy arrays to NodeCollections if such inputs are provided?

Any NodeID can occur only once in a NodeCollection, while repetitions are permitted in arrays. Support for NumPy arrays is mainly aimed at cases where connections are to be generated from explicitly given connection data where repeated NodeIDs will be rather common.

@Silmathoron
Copy link
Member

Silmathoron commented Apr 4, 2020

Any NodeID can occur only once in a NodeCollection, while repetitions are permitted in arrays.

Sure, on the other hand, connection to and from devices have a priori no reason to feature duplicate ids, and an error could simply be raised if the length of the converted NodeCollection differs from that of the initial list...
What I'm trying to say is that the current situation seems unnecessarily restrictive to me.
If there is no strong reason against it, I can provide a PR so that this check and conversion are done automatically (since a check is performed anyway, I can write a single function that checks and converts without degrading code readability or maintainability).

@heplesser heplesser removed ZC: PyNEST DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL labels Apr 7, 2020
@heplesser heplesser added this to the NEST 3.0 milestone Apr 7, 2020
@hakonsbm
Copy link
Contributor Author

hakonsbm commented Apr 7, 2020

Would it be possible to also include the possibility of connecting from and to numpy with devices?
This would require adapting things below L257 in hl_api_connections.py (Connect).

@Silmathoron I'm not sure this is what you're asking about, but when connecting you can use node IDs of devices as well, e.g.:

nest.Create('iaf_psc_alpha', 10)  # Node IDs 1-10
nest.Create('spike_detector')  # Node ID 11
sources = np.array([1, 5, 7], dtype=np.uint64)
targets = np.array([11, 11, 11], dtype=np.uint64)
weights = np.ones(len(sources))
delays = np.ones(len(sources))

nest.Connect(sources, targets, syn_spec={'weight': weights, 'delay': delays,
                                         'synapse_model': 'static_synapse'})

@Silmathoron
Copy link
Member

Silmathoron commented Apr 8, 2020

@hakonsbm @heplesser
Here is a short code example; currently only the first Connect call works.
What I propose is to make all of them work, especially the simpler ones, since having to add the whole syn_spec part makes it very heavy.
Reasons for it:

  • convenience (if you work with graphs, your ids will not be NodeCollection but lists or arrays because this is what graph libraries use, always having to convert back and forth is tedious)
  • consistency (if you can use arrays in one case, it's more intuitive if you can always use them)
import nest
import numpy as np

# create neurons and devices
neuron = nest.Create("iaf_psc_alpha", 10)

vm = nest.Create("voltmeter", 10)

# set weights
weights = np.ones(len(neuron))
delays = np.ones(len(neuron))

# numpy version
arr_vm  = np.array(vm.tolist(), dtype=int)
arr_nrn = np.array(neuron.tolist(), dtype=int)

# works
nest.Connect(arr_vm, arr_nrn, syn_spec={'weight': weights, 'delay': delays, 'synapse_model': 'static_synapse'})

# does not work
nest.Connect(arr_vm, neuron.tolist(), syn_spec={'weight': weights, 'delay': delays, 'synapse_model': 'static_synapse'})
nest.Connect(vm.tolist(), neuron.tolist(), syn_spec={'weight': weights, 'delay': delays, 'synapse_model': 'static_synapse'})

nest.Connect(arr_vm, arr_nrn)
nest.Connect(arr_vm, neuron, syn_spec={'weight': weights, 'delay': delays, 'synapse_model': 'static_synapse'})
nest.Connect(arr_vm, neuron)
nest.Connect(vm.tolist(), neuron)

the way I see it, requiring syn_spec is not necessary and we could either default to the numpy connect if one of the entries is not a NodeCollection, or first check whether conversion to NodeCollection is possible (pretty harmless in my view but maybe I am missing something...) since we're already checking the type anyway (cf. previous message).

@alberto-antonietti
Copy link
Contributor

I stressed a bit the new function :-)

If I try to connect two NumPy arrays where pre and/or post neuronshave a single element, e.g.:

nest.Create('iaf_psc_alpha', 10)
sources = np.array(1, dtype=np.uint64)
targets = np.array(2, dtype=np.uint64)
nest.Connect(sources, targets, syn_spec={'synapse_model': 'static_synapse'})

I get

----> 8 nest.Connect(sources, targets, syn_spec={'synapse_model': 'static_synapse'})

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/ll_api.py in stack_checker_func(*args, **kwargs)
245 def stack_checker_func(*args, **kwargs):
246 if not get_debug():
--> 247 return f(*args, **kwargs)
248 else:
249 sr('count')

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/lib/hl_api_connections.py in Connect(pre, post, conn_spec, syn_spec, return_synapsecollection)
218 # to the right formats.
219 processed_syn_spec = _process_syn_spec(
--> 220 syn_spec, processed_conn_spec, len(pre), len(post), connect_np_arrays)
221
222 # If pre and post are arrays of node IDs, and conn_spec is unspecified,

TypeError: len() of unsized object

Another similar error in the parameters provided can be the following one:

nest.Create('iaf_psc_alpha', 10)
sources = np.array([1], dtype=np.uint64)
targets = np.array([2], dtype=np.uint64)
nest.Connect(sources, targets, syn_spec={'synapse_model': 'static_synapse', 'weight': np.array(1.0)})
# or
nest.Connect(sources, targets, syn_spec={'synapse_model': 'static_synapse', 'delay': np.array(1.0)})

----> 4 nest.Connect(sources, targets, syn_spec={'synapse_model': 'static_synapse', 'weight': np.array(1.0)})

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/ll_api.py in stack_checker_func(*args, **kwargs)
245 def stack_checker_func(*args, **kwargs):
246 if not get_debug():
--> 247 return f(*args, **kwargs)
248 else:
249 sr('count')

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/lib/hl_api_connections.py in Connect(pre, post, conn_spec, syn_spec, return_synapsecollection)
249 syn_param_values = None
250
--> 251 connect_arrays(pre, post, weights, delays, synapse_model, syn_param_keys, syn_param_values)
252 return
253

pynestkernel.pyx in pynestkernel.NESTEngine.connect_arrays()

TypeError: weights must be a NumPy array

I added check for the first case and slightly modified the error messages for the second case.
(see hakonsbm#28)

…from_data

small changes for managing 0-dimensional inputs
Copy link
Contributor

@alberto-antonietti alberto-antonietti left a comment

Choose a reason for hiding this comment

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

Thank you for the effort!
I am now reviewing the related PR #1485

@heplesser
Copy link
Contributor

This fixes #1423 and HBP Ticket #482997.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: External API Developers of extensions or other language bindings may need to adapt their code S: High Should be handled next T: Enhancement New functionality, model or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Support efficient connection generation from data
7 participants