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

New policy 'round_robin_halt' #1868

Merged
merged 11 commits into from
Jun 14, 2022
Merged

Conversation

jlubo
Copy link
Collaborator

@jlubo jlubo commented Mar 24, 2022

Introduces the new policy round_robin_halt. This enables to halt at the current item until the policy round_robin is called again.

Related to issue #1749.

@brenthuisman
Copy link
Contributor

Hi @jlubo, sorry for taking a while to respond. Is it feasible to add a unit test for this?

@jlubo
Copy link
Collaborator Author

jlubo commented May 4, 2022

Okay, I'll try to do that in the next few days.

@jlubo jlubo force-pushed the round_robin_halt branch from 4483daf to 97a09d1 Compare May 10, 2022 21:15
@brenthuisman
Copy link
Contributor

Great, thanks! I see an error (twice) in the run:

ERROR: test_multiple_connections (test.unit.test_multiple_connections.TestMultipleConnections)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/arbor/arbor/python/test/fixtures.py", line 30, in wrapper
    return func(*args, **kwargs)
  File "/home/runner/work/arbor/arbor/python/test/fixtures.py", line 30, in wrapper
    return func(*args, **kwargs)
  File "/home/runner/work/arbor/arbor/python/test/fixtures.py", line 30, in wrapper
    return func(*args, **kwargs)
  File "/home/runner/work/arbor/arbor/python/test/unit/test_multiple_connections.py", line 116, in test_multiple_connections
    data_mem, _ = sim.samples(handle_mem)[0]
IndexError: list index out of range

This is because you are collecting all results on one node. Unfortunately, that won't work; through samples you can only access results that were computed on the node your script is running, see also this page in the docs. At the moment, there is no other solution than to collect the results yourself: i.e. return the results and collect, or write to disk and read back. See the above documentation for how you could handle it.

I admit this isn't great, but for the moment there is no way around it.

@jlubo
Copy link
Collaborator Author

jlubo commented May 11, 2022

Thanks @brenthuisman for pointing that out. To avoid the error in MPI setups, I've now added a condition that prevents running the related assertion (which is anyway not crucial for this test) if the data is not available on the current node. In the future this condition may be removed if #1892 is solved.

@jlubo
Copy link
Collaborator Author

jlubo commented May 12, 2022

The next problem was quite hard to wrap my head around... It seems that the tests (as of commit 417bd5e) did sometimes not pass because the relationship between lid and state in label_resolution.cpp can differ across machines. On my local machine, for example, if round_robin_state.state == 0 then lid.value() == 1. On the machine running the PR check, however, round_robin_state.state == 0 corresponded to lid.value() == 0, causing different synapses to be targeted and leading to different results.

This problem has now (as of commit a8627a5) been solved for the tests by considering instances of synapse mechanisms that are indistinguishable upon initialization (when the lid indices are determined) before evolving differently. In order to test the effect of the round_robin_halt policy, it is necessary to observe such differences between the instances of the same mechanism, and I have now implemented them by using a synapse mechanism that features plasticity (expsyn_stdp).

@brenthuisman brenthuisman requested a review from boeschf May 24, 2022 11:13
@brenthuisman brenthuisman merged commit d06d74a into arbor-sim:master Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants