nest-simulator Revise sp_manager::global

Revise sp_manager::global_shuffle

Open heplesser opened this issue 4 years ago • 4 comments

Currently, the behavior of sp_manager::global_shuffle() does not agree with the comment describing the method:

The comment states that the method "shuffles the first n elements of the vector", implying that the elements from n+1 on remain in the vector in original order
The method randomly picks n elements from the entire vector without replacement and then replaces the content of the vector with these first n elements, dropping the rest

The way the method is used (eg here https://github.com/nest/nest-simulator/blob/5af666418730e3a0a7f7a1c0f717d19dd23a6434/nestkernel/sp_manager.cpp#L439-L440) indicates that the actual behavior is what is intended (but the resize() after calling the method is then not necessary).

Comment and code need to be brought into agreement. One might also consider a more informative method name, including the "chopping" part. The implementation should be reviewed as well. It should be able to work in-place without need of the "erase" function (costly). It might make sense to use suitable C++11 functions, e.g. https://www.cplusplus.com/reference/algorithm/random_shuffle/. Finally, variable names in the method are very uninformative and should be revised.