Fail gracefully when node_id 0 is used to access non-existent node

[nicolossus]

Fixes #2636, fixes #2795.

This issue occurred when no nodes had been created and node_id=0 was provided as an array-like object to Connect or to access a NodeCollection. That is, the following cases would result in a crash instead of an error message:

import nest
nest.Connect([0], [0])    # Crash case 1
nest.NodeCollection([0])  # Crash case 2

Providing a node_id=0 is not really valid and will fail if a NodeCollection has been created. However, if no nodes have been created before trying to connect or access, node_id=0 passes through the consistency check of ModelRangeManager::is_in_range because first_node_id_ and last_node_id_ both have 0 as default values. Instead of introducing two additional checks ( first_node_id_ > 0 and last_node_id_ > 0) to account for this edge case, we can just ensure that node_id > 0 with the same effect.

An alternative solution to this issue could be to ensure that 0 is not contained in array-like objects passed to Connect or NodeCollection on the PyNEST level.

[Helveg]

Thanks! Looks good. I think this solution in is_in_range is probably better fit than at PyNEST. Future C++ API users thank you already :)

[heplesser]

@nicolossus This looks good in principle and should not affect perfomance noticeably. But I noticed that in addition to the is_in_range() you fixed in ModelRangeManager, there is also one in modelrange (

nest-simulator/nestkernel/modelrange.h

Line 37 in 291bdff

is_in_range( size_t node_id ) const

). Could you (a) check how these two are related, (b) if we really need both, and (c) if the second one can either be eliminated to avoid code duplication or needs the same fix?

[nicolossus]

@heplesser The ModelRangeManager class creates an array of modelrange class instances. The is_in_range check is there to safeguard the get_model_id method. In the current implementation, both ModelRangeManager and modelrange needs the is_in_range check and in theory they should be identical. However, since ModelRangeManager::is_in_range is run first with the check for node_id>0, a node_id=0 cannot be passed to modelrange::is_in_range. This means that we strictly don't have to add another condition to check in modelrange::is_in_range. I would perhaps sacrifice the small performance impact of an additional condition for consistency. What do you prefer?