[MRG] fix artificial cell gid assignment for non-cell-specific drives #399

rythorpe · 2021-07-22T20:27:51Z

As noticed in #383, this resolves the discrepancy between MPIBackend and JoblibBackend for simulations that include non-cell-specific drives (e.g., bursty/rhythmic drives).

rythorpe · 2021-07-22T20:31:12Z

hnn_core/network_builder.py

+        n_drive_cells = len(self._drive_cells)
+        print(f'n_art_cells = {n_drive_cells}')


I'm temporarily including these lines to demo the issue. Previously, a simulation using MPIBackend with 6 processes would end up creating at total of 80 drive cells when 20 were expected.

can you think of a way to test the gid assignments? I think you might have to be a little clever how you set it up. E.g., have a parameter called rank or test_mode in one of the functions that fakes the MPIBackend with rank > 1. See how we do something similar in MNE:

https://github.com/mne-tools/mne-python/blob/9b43b2beeca84931049bdc83717aa579a747f2ea/mne/report.py#L94-L97

or maybe have rank as an attribute of NetworkBuilder or Network that is set once somewhere and you can manually tweak it or set it automatically from _get_rank

That's a good idea. I'll give it a shot, though I probably won't be able to get around to it until later next week.

codecov-commenter · 2021-07-22T21:01:09Z

Codecov Report

Merging #399 (ca5fc1b) into master (f9358d8) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head ca5fc1b differs from pull request most recent head 2632318. Consider uploading reports for the commit 2632318 to get more accurate results

@@            Coverage Diff             @@
##           master     #399      +/-   ##
==========================================
+ Coverage   90.13%   90.16%   +0.03%     
==========================================
  Files          17       17              
  Lines        3050     3060      +10     
==========================================
+ Hits         2749     2759      +10     
  Misses        301      301

Impacted Files	Coverage Δ
hnn_core/network.py	`92.00% <100.00%> (ø)`
hnn_core/network_builder.py	`94.13% <100.00%> (+0.20%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9358d8...2632318. Read the comment docs.

rythorpe · 2021-07-30T21:56:15Z

I gave your suggestion a try in 02b6e60 @jasmainak, but am now encountering the following error:

test_parallel_backends.py F.........                                                          [100%]

============================================= FAILURES ==============================================
_________________________________ test_gid_assignment_across_ranks __________________________________

    def test_gid_assignment_across_ranks():
        """Test that gids are assigned without overlap across ranks"""
        hnn_core_root = op.dirname(hnn_core.__file__)
        params_fname = op.join(hnn_core_root, 'param', 'default.json')
        params = read_params(params_fname)
        params.update({'N_pyr_x': 3,
                       'N_pyr_y': 3,
                       'tstop': 40,
                       't_evprox_1': 5,
                       't_evdist_1': 10,
                       't_evprox_2': 20,
                       'N_trials': 2})
        net = jones_2009_model(params, add_drives_from_params=True)
        n_drive_cells = {name: drive['n_drive_cells'] for name, drive in
                         net.external_drives.items()}
        n_ranks = 3
        n_drive_cells_instantiated = dict()
        for rank in range(n_ranks):
            net_builder = NetworkBuilder(net)
            net_builder._gid_list = list()
            net_builder._cells = list()
            net_builder._drive_cells = list()
>           net_builder._build(test_rank=rank)

test_parallel_backends.py:58: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../network_builder.py:325: in _build
    self._create_cells_and_drives(threshold=self.net._params['threshold'],
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <hnn_core.network_builder.NetworkBuilder object at 0x7f0938f04190>, threshold = 0.0
record_vsoma = 0, record_isoma = 0

    def _create_cells_and_drives(self, threshold, record_vsoma=False,
                                 record_isoma=False):
        """Parallel create cells AND external drives
    
        NB: _Cell.__init__ calls h.Section -> non-picklable!
        NB: _ArtificialCell.__init__ calls h.*** -> non-picklable!
    
        These drives are spike SOURCES but cells are also targets.
        External inputs are not targets.
        """
        # loop through ALL gids
        # have to loop over self._gid_list, since this is what we got
        # on this rank (MPI)
        for gid in self._gid_list:
            src_type = self.net.gid_to_type(gid)
            cell = self.net._gid_to_cell(gid)
    
            if cell is not None:
                # instantiate NEURON object
                if src_type in ('L2_pyramidal', 'L5_pyramidal'):
                    cell.build(sec_name_apical='apical_trunk')
                else:
                    cell.build()
                # add tonic biases
                if ('tonic' in self.net.external_biases and
                        src_type in self.net.external_biases['tonic']):
                    cell.create_tonic_bias(**self.net.external_biases
                                           ['tonic'][src_type])
                cell.record_soma(record_vsoma, record_isoma)
    
                # this call could belong in init of a _Cell (with threshold)?
                nrn_netcon = cell.setup_source_netcon(threshold)
>               _PC.cell(cell.gid, nrn_netcon)
E               RuntimeError: hoc error

../network_builder.py:432: RuntimeError
--------------------------------------- Captured stdout call ----------------------------------------
Loading custom mechanism files from /home/ryan/hnn-core/hnn_core/mod/x86_64/.libs/libnrnmech.so
Building the NEURON model
n_art_cells = 140
[Done]
Building the NEURON model
n_art_cells = 140
[Done]
Building the NEURON model
n_art_cells = 140
[Done]
--------------------------------------- Captured stderr call ----------------------------------------
NEURON: gid=1 has not been set on rank 0
 near line 0
 objref hoc_obj_[2]
                   ^
        ParallelContext[0].cell(1, ...)

Do you have any idea what's going on or know of a better way to structure this test?

jasmainak · 2021-07-31T01:17:42Z

hnn_core/network_builder.py

+        for gid in self._gid_list:
+            _PC.set_gid2node(gid, rank)


@rythorpe see how I separated the Neuron code. Now you can test the code above without making a call to Neuron

Ahh yes, now I see it. Thanks @jasmainak!

jasmainak · 2021-07-31T01:18:53Z

hnn_core/tests/test_parallel_backends.py

+    n_drive_cells_instantiated = dict()
+    for rank in range(n_ranks):
+        net_builder = NetworkBuilder(net)
+        net_builder._build(test_rank=rank)


just test _gid_assign, not the whole _build ... and separate out the Neuron calls because you don't really have rank > 0. You are trying to create a fake rank

jasmainak · 2021-07-31T01:21:49Z

@rythorpe take a look at aff49b5

you need to create a "fake rank" for testing. If Neuron is called, then you are screwed because rank > 0 does not really exist. See how the rank reported by Neuron (0) does not match self._rank (1)

hnn_core/tests/test_parallel_backends.py

jasmainak · 2021-07-31T01:39:20Z

hnn_core/tests/test_parallel_backends.py

+                   't_evdist_1': 10,
+                   't_evprox_2': 20,
+                   'N_trials': 2})
+    net = jones_2009_model(params, add_drives_from_params=True)


Suggested change

net = jones_2009_model(params, add_drives_from_params=True)

net = jones_2009_model(params, add_drives_from_params=False)

you might want to use _gid_list for the test. Add a drive explicitly and check if it contains all the gids in one rank if it's non-cell-specific etc.

Done. See cce3bd9.

ntolley · 2021-08-03T21:03:50Z

hnn_core/network.py

@@ -330,7 +330,7 @@ def _update_cells(self):
                    cell.gid = self.gid_ranges[cell_type][cell_idx]
                    cell.pos = pos
                    cells.append(cell)
-                    self.cells[cell_type] = cells
+                self.cells[cell_type] = cells


Subtle, assuming this is a silent fix where the final behavior is the same?

Correct. When I was trying to find the root of the discrepany between the two parallel backends, I realized that we don't need to assign the list of cell objects to the net.cells dict every time it iterates through the innermost loop. Just a super minor performance improvement.

hnn_core/network_builder.py

rythorpe · 2021-08-05T03:14:14Z

Aside from the MPI test failing as in #406, I think this is ready to go. Any other suggestions @jasmainak and @ntolley?

jasmainak · 2021-08-10T12:00:15Z

Is the MPI test failing consistently? I just restarted the CI so hopefully it passes. We can't merge PRs unless all tests pass ...

jasmainak

LGTM @ntolley do you want to merge once the CIs are green?

jasmainak · 2021-08-10T12:07:57Z

@rythorpe what's our plans with this example: https://1901-168215891-gh.circle-artifacts.com/0/html/auto_examples/howto/plot_simulate_mpi_backend.html#sphx-glr-auto-examples-howto-plot-simulate-mpi-backend-py ? Is that good as it is or do we want to use a seed to make it look more sinusoidal as it was before?

examples/workflows/plot_simulate_beta.py

hnn_core/network_builder.py

hnn_core/tests/test_parallel_backends.py

ntolley · 2021-08-10T13:42:12Z

@rythorpe appologies for the late test suggestion! Happy to merge after addressing the remaining questions

rythorpe · 2021-08-10T20:22:55Z

@rythorpe what's our plans with this example: https://1901-168215891-gh.circle-artifacts.com/0/html/auto_examples/howto/plot_simulate_mpi_backend.html#sphx-glr-auto-examples-howto-plot-simulate-mpi-backend-py ? Is that good as it is or do we want to use a seed to make it look more sinusoidal as it was before?

How do you feel about this simulation with seedcore=8? The later peaks aren't as extreme but the signal looks more clearly like an alpha oscillation (at least to my eye).

ntolley · 2021-08-10T23:53:02Z

Happy to merge, but for some reason github actions wasn't triggered for the test suite?

rythorpe · 2021-08-11T01:34:52Z

Happy to merge, but for some reason github actions wasn't triggered for the test suite?

That's curious. Do you know how to force them to run?

rythorpe · 2021-08-11T02:52:03Z

Go for it @ntolley!

jasmainak · 2021-08-11T02:54:47Z

good stuff, happy how this PR shaped up!

rythorpe added the bug Something isn't working label Jul 22, 2021

rythorpe commented Jul 22, 2021

View reviewed changes

jasmainak added this to the 0.2 milestone Jul 28, 2021

jasmainak reviewed Jul 31, 2021

View reviewed changes

hnn_core/tests/test_parallel_backends.py Outdated Show resolved Hide resolved

jasmainak reviewed Jul 31, 2021

View reviewed changes

jasmainak force-pushed the gid_assign branch from 35740f2 to 6226b23 Compare July 31, 2021 03:50

ntolley reviewed Aug 3, 2021

View reviewed changes

hnn_core/network_builder.py Show resolved Hide resolved

rythorpe and others added 5 commits August 4, 2021 21:15

fix artificial cell gid assignment for non-cell-specific drives

8e4057b

change seedcore for bursty drives

7b8a53d

add test

bdf9798

Cosmit

071f846

MAINT: Cleanup

6430b2a

rythorpe force-pushed the gid_assign branch from 6226b23 to 6430b2a Compare August 5, 2021 01:20

rythorpe added 3 commits August 4, 2021 22:50

test gid assignment using NetworkBuilder._gid_list

cce3bd9

remove print statement

f575cb4

explicitly define drives in test_gid_assignment

af61396

whats_new.rst

0acc210

rythorpe changed the title ~~[WIP] fix artificial cell gid assignment for non-cell-specific drives~~ [MRG] fix artificial cell gid assignment for non-cell-specific drives Aug 5, 2021

jasmainak approved these changes Aug 10, 2021

View reviewed changes

ntolley reviewed Aug 10, 2021

View reviewed changes

examples/workflows/plot_simulate_beta.py Show resolved Hide resolved

ntolley reviewed Aug 10, 2021

View reviewed changes

hnn_core/network_builder.py Show resolved Hide resolved

ntolley reviewed Aug 10, 2021

View reviewed changes

hnn_core/tests/test_parallel_backends.py Outdated Show resolved Hide resolved

rythorpe added 2 commits August 10, 2021 16:23

adjust plot_simulate_mpi_backend.py seedcore value

30a32bb

test for gid overlap explicitly

2632318

ntolley approved these changes Aug 10, 2021

View reviewed changes

rythorpe force-pushed the gid_assign branch from ca5fc1b to 2632318 Compare August 11, 2021 02:36

ntolley merged commit 63518fe into jonescompneurolab:master Aug 11, 2021

rythorpe deleted the gid_assign branch August 11, 2021 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] fix artificial cell gid assignment for non-cell-specific drives #399

[MRG] fix artificial cell gid assignment for non-cell-specific drives #399

rythorpe commented Jul 22, 2021

rythorpe Jul 22, 2021

jasmainak Jul 23, 2021 •

edited

rythorpe Jul 23, 2021

codecov-commenter commented Jul 22, 2021 •

edited

rythorpe commented Jul 30, 2021

jasmainak Jul 31, 2021

rythorpe Aug 5, 2021

jasmainak Jul 31, 2021

jasmainak commented Jul 31, 2021 •

edited

jasmainak Jul 31, 2021

rythorpe Aug 5, 2021

ntolley Aug 3, 2021

rythorpe Aug 5, 2021

rythorpe commented Aug 5, 2021

jasmainak commented Aug 10, 2021

jasmainak left a comment

jasmainak commented Aug 10, 2021

ntolley commented Aug 10, 2021

rythorpe commented Aug 10, 2021

ntolley commented Aug 10, 2021

rythorpe commented Aug 11, 2021

rythorpe commented Aug 11, 2021

jasmainak commented Aug 11, 2021

		n_drive_cells = len(self._drive_cells)
		print(f'n_art_cells = {n_drive_cells}')

	net = jones_2009_model(params, add_drives_from_params=True)
	net = jones_2009_model(params, add_drives_from_params=False)

[MRG] fix artificial cell gid assignment for non-cell-specific drives #399

[MRG] fix artificial cell gid assignment for non-cell-specific drives #399

Conversation

rythorpe commented Jul 22, 2021

Choose a reason for hiding this comment

jasmainak Jul 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 22, 2021 • edited

Codecov Report

rythorpe commented Jul 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak commented Jul 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rythorpe commented Aug 5, 2021

jasmainak commented Aug 10, 2021

jasmainak left a comment

Choose a reason for hiding this comment

jasmainak commented Aug 10, 2021

ntolley commented Aug 10, 2021

rythorpe commented Aug 10, 2021

ntolley commented Aug 10, 2021

rythorpe commented Aug 11, 2021

rythorpe commented Aug 11, 2021

jasmainak commented Aug 11, 2021

jasmainak Jul 23, 2021 •

edited

codecov-commenter commented Jul 22, 2021 •

edited

jasmainak commented Jul 31, 2021 •

edited