Fix merging responses in nest-server-mpi
#3492
Conversation
babsey
force-pushed
the
nest-server-mpi-merge-responses
branch
from
78404ae to
909e5eb
Compare
babsey
changed the title
nest-server-mpi
gtrensch
added
T: Bug
heplesser
added
S: High
|
I am working on better solution of combine function. Thank you for the patience. |
…est-simulator into nest-server-mpi-merge-responses
| * for some specific calls, the responses are known to be the | ||
| same from the master and all workers. In this case, the | ||
| combined response is just the master response | ||
| be None. |
There was a problem hiding this comment.
Since all bullet points begin with lowercase letters, all bullets except the last should end with ;
| * the responses are known to be the same from the master and all | ||
| workers. In this case, the combined response is just the master | ||
| response. |
There was a problem hiding this comment.
Please avoid loaded terms such as "master" and "workers". All MPI ranks in a NEST simulation are doing the same work, just on different parts of the network. Also, how would it be "known" that we get the same response from each rank?
| * if the response contains one list per process, the combined | ||
| response will be those first list. |
There was a problem hiding this comment.
I don't understand this, especially the "those first list". If I get one list from each rank and the data fall not under the "same from all" clause above, don't I need to combine the lists from all ranks?
| if len(filtered_response) == 1: | ||
| return filtered_response[0] | ||
| # return first list/tuple if the response only consists of lists or tuples. | ||
| elif all(type(v) in [list, tuple] for v in response): |
There was a problem hiding this comment.
Wouldn't
| elif all(type(v) in [list, tuple] for v in response): | |
| elif all(isinstance(v, (list, tuple)) for v in response): |
be more elegant?
| def merge_dicts(response): | ||
| """Merge status dictionaries of recorders | ||
| def merge_dicts(response: list[dict]) -> dict: | ||
| """Merge dictionaries of the response. |
There was a problem hiding this comment.
Why are you removing the documentation below. merge_dicts() seems to do rather complex things depending on the structure of its input. Without a docstring explaining what the intended result is for the different cases, I find it difficult to review the code.
When executing simulation via
nest-server-mpi, the result seems not correctly.In fact, when using a number of processes greater than 1, lets say an example
np = 3.It shows only activity of each 3rd neuron.
It seems that we need to merge the responses of
nest-server-mpi.With this PR, NEST Desktop shows activity of all neurons executed on
nest-server-mpi.