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.

Sign up for GitHub

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

Jump to bottom

[MRG] fix inconsistent random state assignment between parallel trials #171

Merged
merged 6 commits into from Sep 15, 2020
Merged

[MRG] fix inconsistent random state assignment between parallel trials #171

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bc59634
update feed-specific param dictionaries in Network attribute of Neuro…
rythorpe Aug 21, 2020
dfa6445
fix trial message for MPI_BACKEND
rythorpe Sep 11, 2020
7118e29
iterate rng seed param for each trial in mpi_child
rythorpe Sep 11, 2020
b340fe0
MPI and Joblib backends now produce the same results
rythorpe Sep 14, 2020
b8f5d1d
add comparison tests across parallel backends
rythorpe Sep 15, 2020
98a4050
update whats_new.rst
rythorpe Sep 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions hnn_core/mpi_child.py
View file
Open in desktop
Expand Up @@ -60,10 +60,15 @@ def run_mpi_simulation():

params = comm.bcast(params, root=0)
net = Network(params)
neuron_net = NetworkBuilder(net)

sim_data = []
for trial in range(params['N_trials']):
for trial_idx in range(params['N_trials']):
# update rng seed state with new trial
#if rank == 0:
net.trial_idx = trial_idx
for param_key in net.params['prng_*'].keys():
net.params[param_key] += trial_idx
neuron_net = NetworkBuilder(net)
dpl = _simulate_single_trial(neuron_net)
if rank == 0:
spikedata = neuron_net.get_data_from_neuron()
Expand Down
8 changes: 6 additions & 2 deletions hnn_core/network_builder.py
View file
Open in desktop
Expand Up @@ -11,6 +11,7 @@
from .cell import _ArtificialCell
from .pyramidal import L2Pyr, L5Pyr
from .basket import L2Basket, L5Basket
from .params import create_pext

# a few globals
_PC = None
Expand Down Expand Up @@ -121,8 +122,6 @@ def simulation_time():
dpl.scale(neuron_net.net.params['dipole_scalefctr'])
dpl.smooth(neuron_net.net.params['dipole_smooth_win'] / h.dt)

neuron_net.net.trial_idx += 1

return dpl


Expand Down Expand Up @@ -353,6 +352,11 @@ def _create_cells_and_feeds(self):
External inputs are not targets.
"""
params = self.net.params
# Re-create external feed param dictionaries
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add an "XXX" in the comment somewhere? To indicate that this is a hack (right?) and we should get rid of at some point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure in this scenario what delineates a hack from a long-term bug fix. We plan to refactor the params and feed modules wrt how feed objects and their respective parameters are passed to Network and NetworkBuilder. Along the way we will end up simplifying the code by e.g. removing the need for calling create_pext() here. That said, I'm happy to update the comment if you think it'd be clearer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I agree it might go away but just leaving an XXX tells future contributors that it's a hack that needs to fixed.

# Note that the only thing being updated here are the param['prng_*']
# values
self.net.p_common, self.net.p_unique = create_pext(params,
params['tstop'])

# loop through gids on this node
for gid in self.net._gid_list:
Expand Down
5 changes: 3 additions & 2 deletions hnn_core/parallel_backends.py
View file
Open in desktop
Expand Up @@ -86,10 +86,11 @@ def _clone_and_simulate(self, net, trial_idx):
from hnn_core.network_builder import NetworkBuilder
from hnn_core.network_builder import _simulate_single_trial

if trial_idx != 0:
net.params['prng_*'] = trial_idx
for param_key in net.params['prng_*'].keys():
net.params[param_key] += trial_idx

neuron_net = NetworkBuilder(net)
neuron_net.net.trial_idx = trial_idx
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this equivalent to https://github.com/jonescompneurolab/hnn-core/pull/171/files#diff-ca5f146e027d9dc6ef29ff4db9c21ca2R68 ? I have a suspicion it is not. Can you check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I messed this up. Fixed in b340fe0.

dpl = _simulate_single_trial(neuron_net)

spikedata = neuron_net.get_data_from_neuron()
Expand Down