Add array indexing to NodeCollections #1485

hakonsbm · 2020-03-27T12:47:26Z

This PR adds the ability to index NodeCollections with arrays of integers or bools, similar to advanced indexing in NumPy. The result is a new NodeCollection with the selected nodes. Note that a NodeCollection can only contain unique node IDs. Array indexing supports lists, tuples, and NumPy arrays as indices.

Example:

n = nest.Create('iaf_psc_alpha', 5)
n[[1, 2]]  # -> NodeCollection with node IDs 2 and 3
n[(0, 1, 3, 4)]  # -> NodeCollection with node IDs 1, 2, 4, and 5
n[[True, False, True, False, True]]  # -> NodeCollection with node IDs 1, 3, and 5
n[[2, 3, 3, 4]]  # Error: NodeCollection cannot contain non-unique node IDs

alberto-antonietti

Thank you for adding these additional features to better integrate NEST with NumPy arrays!

I have added some minor comments inline.

Another comment not strictly related to your changes.
When working with lists or arrays, if an index outside of the NodeCollection is used, then the right error is thrown:

n = nest.Create('iaf_psc_alpha', 10) 
n[[1, 5, 10]]

IndexError: pos points outside of the NodeCollection

nest-simulator/nestkernel/node_collection.h

Line 813 in 8bb59ac

throw std::out_of_range( "pos points outside of the NodeCollection" );

This does not happen when using integers or slices as indexes:

n = nest.Create('iaf_psc_alpha', 10) 
n[10]
# OR
n[1:12]

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/lib/hl_api_types.py in getitem(self, key)
216 step = 1 if key.step is None else key.step
217
--> 218 return sli_func('Take', self._datum, [start, stop, step])
219 elif isinstance(key, (int, numpy.integer)):
220 return sli_func('Take', self._datum, [key + (key >= 0)])

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/ll_api.py in sli_func(s, *args, **kwargs)
181 sli_push(args) # push array of arguments on SLI stack
182 sli_push(s) # push command string
--> 183 sli_run(slifun) # SLI support code to execute s on args
184 r = sli_pop() # return value is an array
185

~/workspace/nest-simulator/b/lib/python3.6/site-packages/nest/ll_api.py in catching_sli_run(cmd)
131
132 exceptionCls = getattr(kernel.NESTErrors, errorname)
--> 133 raise exceptionCls(commandname, message)
134
135

NESTErrors.BadParameter: ('BadParameter in Take_g_a: stop <= size() required.', 'BadParameter', 'Take_g_a', ': stop <= size() required.')

I do not know if this PR is the place where to fix this inconsistency. Otherwise, I can create an ad-hoc PR for this.

alberto-antonietti · 2020-04-16T08:28:29Z

pynest/pynestkernel.pyx

+                new_nc_datum = node_collection_array_index(nc_datum, array_long_ptr, len(array))
+                return sli_datum_to_object(new_nc_datum)
+            else:
+                raise TypeError('array must be a NumPyArray of ints or bools, got {}'.format(array.dtype))


typo. "NumPyArray" -> "NumPy array"

alberto-antonietti · 2020-04-16T08:38:37Z

pynest/pynestkernel.pyx

+
+        if not (isinstance(node_collection, SLIDatum) and (<SLIDatum> node_collection).dtype == SLI_TYPE_NODECOLLECTION.decode()):
+            raise TypeError('node_collection must be a NodeCollection, got {}'.format(type(node_collection)))
+        if not isinstance(array, numpy.ndarray):


Should we check here that array.ndim == 1 ?

Good point, I have added it.

alberto-antonietti · 2020-04-16T08:46:41Z

pynest/pynestkernel.pyx

+        if not (isinstance(node_collection, SLIDatum) and (<SLIDatum> node_collection).dtype == SLI_TYPE_NODECOLLECTION.decode()):
+            raise TypeError('node_collection must be a NodeCollection, got {}'.format(type(node_collection)))
+        if not isinstance(array, numpy.ndarray):
+            raise TypeError('array must be a NumPy array of ints or bools, got {}'.format(type(array)))


If the above comment is addressed, you could add:

if not array.ndim == 1: raise TypeError('array must be a 1-dimensional NumPy array, got {}-dimensional NumPy array'.format(array.ndim))

alberto-antonietti · 2020-04-16T12:30:20Z

nestkernel/nest.cpp

+  std::vector< index > node_ids;
+  node_ids.reserve( n );
+
+  for ( auto node_ptr = array; node_ptr != array + n; ++node_ptr )


Is there a reason why you are using auto instead of long* ?

The idea is that we don't care about the type of node_ptr as long as it's the same as the type of array. If the type of array is changed, it only has to change in the definition of the function. Ensuring that they always have the same type can avoid some nasty bugs down the line if the type of array is changed.

OK, got it.

alberto-antonietti · 2020-04-16T12:31:10Z

nestkernel/nest.cpp

+  node_ids.reserve( n );
+
+  auto nc_it = node_collection->begin();
+  for ( auto node_ptr = array; node_ptr != array + n; ++node_ptr, ++nc_it )


Same as above: Is there a reason why you are using auto instead of bool*?

Same as above.

alberto-antonietti · 2020-04-16T12:46:44Z

nestkernel/node_collection.cpp

@@ -158,7 +170,7 @@ NodeCollection::create_( const std::vector< index >& node_ids )

  std::vector< NodeCollectionPrimitive > parts;

-  index old_node_id = 0;
+  index old_node_id = current_first;


This change is not relative to the array indexing introduced with this PR, right?

I assume it solves the bug that was not checking the repetition of the first element in the list. Something like nest.NodeCollection([2, 2, 3, 4]) was not throwing errors before.

True, it's not relevant for the array indexing. I can take it out and create a separate PR for it.

The changes are now in #1536.

jougs · 2020-04-22T08:07:36Z

@hakonsbm: I would only review once you addressed the comments by @alberto-antonietti and fixed the conflicts, OK?

hakonsbm · 2020-04-22T09:37:22Z

@alberto-antonietti Thanks for your review! I have addressed most of your comments, still working on the last one.

Another comment not strictly related to your changes.
When working with lists or arrays, if an index outside of the NodeCollection is used, then the right error is thrown:

(...)

I do not know if this PR is the place where to fix this inconsistency. Otherwise, I can create an ad-hoc PR for this.

I agree the error message could be more informative here. If you create a PR for it, I'll be happy to review it.

It is moved to a separate branch.

alberto-antonietti

Thank you for the changes, I am going to take care of the PR for the error message.

Silmathoron

lgtm! 👍

hakonsbm added 11 commits January 28, 2020 11:00

Added test for NodeCollection array indexing

aec1705

Added support for NodeCollection array indexing

1a01067

Refactored NodeCollection array indexing

327921f

Added NodeCollection bool array indexing

735557b

Added more checks and error handling to NodeCollection array indexing

a172c23

More tests of NodeCollection array indexing

43a523e

Formatting cleanup

2fdbcf3

Some minor adjustments to take_array_index

b0021b8

Merge remote-tracking branch 'nest/master' into nest3/nc_array_indexing

3742cb0

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

273879c

Correctly raise error when creating NodeCollection with non-unique IDs

8bb59ac

heplesser requested review from jougs and alberto-antonietti March 31, 2020 14:02

heplesser assigned hakonsbm Apr 7, 2020

heplesser removed ZC: PyNEST DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL labels Apr 7, 2020

heplesser linked an issue Apr 7, 2020 that may be closed by this pull request

Support efficient connection generation from data #1423

Closed

heplesser removed a link to an issue Apr 7, 2020

Support efficient connection generation from data #1423

Closed

alberto-antonietti suggested changes Apr 16, 2020

View reviewed changes

hakonsbm added 2 commits April 22, 2020 10:30

Merge branch 'master' into nest3/nc_array_indexing

c8936ed

Corrected and added checks of array indexing argument

e867b52

Reverted change of NodeCollection creation

a3cc3ed

It is moved to a separate branch.

alberto-antonietti approved these changes Apr 22, 2020

View reviewed changes

hakonsbm mentioned this pull request Apr 22, 2020

Fix NodeCollection creation to correctly raise error when using non-unique or unsorted node IDs #1536

Merged

alberto-antonietti mentioned this pull request Apr 22, 2020

Handling of out-of-range slicing and indexes for NodeCollections #1538

Merged

Silmathoron approved these changes Apr 24, 2020

View reviewed changes

heplesser removed the request for review from jougs April 24, 2020 13:54

heplesser merged commit 15986ce into nest:master Apr 24, 2020

hakonsbm deleted the nest3/nc_array_indexing branch April 24, 2020 13:55

Silmathoron mentioned this pull request Apr 29, 2020

Implicit reordering with array slicing of NodeCollection #1563

Closed

sarakonradi mentioned this pull request Apr 29, 2020

Connect all-to-all with ommiting zero weights? #1353

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add array indexing to NodeCollections #1485

Add array indexing to NodeCollections #1485

hakonsbm commented Mar 27, 2020

alberto-antonietti left a comment

alberto-antonietti Apr 16, 2020

hakonsbm Apr 22, 2020

alberto-antonietti Apr 16, 2020

hakonsbm Apr 22, 2020

alberto-antonietti Apr 16, 2020

alberto-antonietti Apr 16, 2020

hakonsbm Apr 22, 2020

alberto-antonietti Apr 22, 2020

alberto-antonietti Apr 16, 2020

hakonsbm Apr 22, 2020

alberto-antonietti Apr 16, 2020

hakonsbm Apr 22, 2020

hakonsbm Apr 22, 2020

jougs commented Apr 22, 2020

hakonsbm commented Apr 22, 2020

alberto-antonietti left a comment

Silmathoron left a comment

Add array indexing to NodeCollections #1485

Add array indexing to NodeCollections #1485

Conversation

hakonsbm commented Mar 27, 2020

alberto-antonietti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jougs commented Apr 22, 2020

hakonsbm commented Apr 22, 2020

alberto-antonietti left a comment

Choose a reason for hiding this comment

Silmathoron left a comment

Choose a reason for hiding this comment