[MRG] Refactor API for creating external network drives #221

cjayb · 2020-11-30T15:42:00Z

A new API for external inputs to the network, both 'driving' and 'biasing' types. See #222 for a discussion on the nomenclature

The gist of this proposal is to separate the definition and instantiation of driving (and biasing) inputs to the network. 'Drives' are synaptic mechanisms, whereas 'biases' are (currently) applied directly to the model cells (e.g., 'tonic' current clamp to the soma).

Closes #222
Closes #228
Closes #238
(follow-ups at #230 #233)

Drives are defined by the number and timing of synaptic events, in combination with information on connectivity (which synapses are targeted). Because the former are defined in terms of statistics, the proposed API is a two-stage process

attach drives to the network
instantiate the synaptic event time distribution based on the parameters in 1

In the case of multiple trials, 2. is called multiple times with different random number generator seeds.

net = Network(params)
net.add_evoked_drive('evdist1', mu=20., sigma=2., ...)
net.add_poisson_drive('extpois', t0=0., T=params['tstop'], ...)

Progress:

reproduce HNN GUI behaviour and past tests using two-stage API
rename variables and functions to get rid of 'feeds' (may be different PR)
- though feeds.py and test_feeds.py need to be weeded out
replace net._p_common and net._p_unique with less idiosyncratic methods (currently needed because they define connectivity in NetworkBuilder
- NB it does not make sense to define event times alone: the network connectivity is baked into a 'drive'
- ~~should we consider a NeuroML-type specification format?~~
add a new example showing the use of the new API
- ~~add tests and examples for tonic biases~~ (moved, see add example for tonic inputs #223 and [MRG] DOC: use morlet instead of multitaper #226)
- add new example showing how drives can be visualised (and tweaked) before building the network (open new issue once merged)
figure out when to "instantiate" event times
- modified simulate_dipole logic to ensure n_trials>0
- allows changes (e.g., net.external_drives['evdist1']['dynamics']['sigma'] = 0) to be effective (included in plot_simulate_evoked.py with new Network.copy()-method)
- defer discussion on whether Network should have an n_trials-argument to future PR (semantically, the number of trials is a feature of the simulation, not the network)
expose bursty parameters
- numspikes should be allowed to be other than 1 or 2
- spike ISI within a burst should be settable (now fixed at 10 ms)
modify all plot-examples to use new API
set initialise_hnn_drives=False as default for Network.__init__ and change all tests to set it to True
rebase onto master (merge MRG: improve parallel backends #211 later)

jasmainak · 2020-11-30T22:03:44Z

Sorry I have been slow with the tonic PR ... have been helping folks in other PRs. Can help you rebase later :)

I propose that we aim for a release after this PR and the few small PRs needed for GUI integration. We can still add a warning that the version is semi-stable (alpha, beta whatever version) and there might be breaking API changes in the future but having hnn-core on pip could be a great step forward. Let's aim for an Xmas release!

hnn_core/network.py

hnn_core/network_builder.py

hnn_core/tests/conftest.py

hnn_core/tests/test_network.py

cjayb · 2020-12-08T18:11:42Z

@jasmainak @rythorpe @ntolley ready for feedback (code review also welcome, though many things still in flux...). See also updated aims.

hnn_core/network.py

jasmainak · 2020-12-08T19:22:48Z

@cjayb not too thrilled by the new methods :( Could we perhaps zoom so I can understand better what you're trying to achieve? I may be misunderstanding the goals. We need to simplify, not overengineer. What we need is:

net.add_poisson_input(...)
net.add_evoked_input(...)
net.add_rhythmic_input(...)

and these functions will create feed_times. Then instead of create_pext you need to call these methods explicitly with explicit parameters. The danger of adding new attributes and methods which don't explicitly take parameters is that you will get spaghetti bolognese code before you realize it.

cjayb · 2020-12-08T20:08:09Z

Happy to discuss :) Send me an email with a few time proposals in the coming days! Try writing out your add_*-methods though: they end up duplicating everything but the event time stats. The idea behind the 'attach'-concept is that all drives have similarly-defined stats and connectivity. Focusing on the actual 'instantiated' event times in the API is IMHO wrong, or at least misses the point. I'm sure there's a way to use your semantics too, let's talk.

hnn_core/cell.py

hnn_core/network.py

cjayb · 2020-12-14T19:54:17Z

Update: close but no cigar... After much shuffling around, I thought I finally got NetworkBuilder working again. The first 'common' drive ("bursty_0" , gid 270) passes through, but the second one ("bursty_0" , gid 271) dies with the below. Need to take a step back...

@jasmainak @rythorpe would you take a look at Network to see if you roughly speaking approve of how the API is shaping? Take a look under the temporary if initialise_hnn_drives:-bit for syntax.

----------------------------- Captured stdout call -----------------------------
Loading custom mechanism files from /Users/cjb/src/git/hnn-core/hnn_core/mod/x86_64/.libs/libnrnmech.so
Building the NEURON model
oc_restore_code tobj_count=1 should be 0
Traceback (most recent call last):
  File "/Users/cjb/src/git/hnn-core/hnn_core/network_builder.py", line 501, in _connect_celltypes
    gid_src, nc_dict, target_cell.synapses[syn_key])
  File "/Users/cjb/src/git/hnn-core/hnn_core/cell.py", line 346, in parconnect_from_src
    nc = _PC.gid_connect(gid_presyn, postsyn)
RuntimeError: hoc error

----------------------------- Captured stderr call -----------------------------
NEURON: gid 271 owned by 0 but no associated cell
 near line 0
 objref hoc_obj_[2]
                   ^
        ParallelContext[0].gid_connect(271, ...)

hnn_core/network.py

cjayb · 2021-01-17T12:38:59Z

Addressed:

_NetworkDrive made private, removed from API, improved (?) __repr__
Network.copy now tested and added to whats_new
issue Refactor feed into drives using new API calls, and improve transparency of _instantiate_drives #233 updated to indicate next steps for moving from feed to drives

jasmainak

@ntolley @rythorpe dare to merge? :)

or do you want me to do the honors?

hnn_core/drives.py

hnn_core/network.py

ntolley · 2021-01-17T21:40:44Z

hnn_core/network.py

+            Synaptic delay (in ms) at the column origin, dispersed laterally as
+            a function of the space_constant
+        cell_specific : bool
+            Whether each cell has unique connection parameters (default: True)


It's really the cell type though right? As in this doesn't support single gid targeting... yet!

~~Correct. I was on the fence on this one, opted for the short (but misleading) name. Changing to cell_type_specific for clarity.~~

Oops, spoke too early... I actually do mean cell-specific here! This is the old p_unique (vs. p_common) distinction. The flag controls whether each (real) cell in the network gets 'specific' input from a single artificial drive cell, or whether there is a one 'global' drive targeting all cells. In the latter case, the spike dynamics are identical across cells, though the connection strength and synaptic delay may differ (at the level of cell types, not individual cells, as you point out). Make sense?

ntolley · 2021-01-17T21:47:39Z

hnn_core/network.py

+                    if isinstance(synaptic_delays, float):
+                        drive_conn_by_cell[cellname][receptor][
+                            'A_delay'] = synaptic_delays
+                    else:


Maybe elif isinstance(synaptic delays, dict)?

Made the change, though there's a test to ensure it's either a float or a dict (so not including an else: raise ValueError here.

ntolley · 2021-01-17T21:56:46Z

hnn_core/network.py

+                             f', got {cell_type}, {amplitude}')
+        if 'tonic' not in self.external_biases:
+            self.external_biases['tonic'] = dict()
+        if cell_type in self.external_biases['tonic']:


Hmm so does this still get triggered when you copy the network? Or does _reset_drives() take care of that. If not, it may be nice to overwrite specific drives after copying the Network when writing code for parameter sweeps.

See my comment to @rythorpe below. The only thing _reset are previous simulation results, specifically: vsoma/isoma and the 'events' values (spike times) of drives. It does not remove drives or biases.

I think this is what you want when doing a parameter sweep, right?

So if a user first creates a tonic input that targets L2/3 basket cells by calling net.add_tonic_bias(), and then post-hoc decides to change the strength of the injected current for this input, how are they supposed to do this? If I understand things correctly, they can't call net.add_tonic_bias() again because the L2/3 basket cell type already exists in net.external_biases['tonic'] right? Alternatively, I guess the user could directly modify net.external_biases['tonic'], but if this is the case it should say so in the documentation explicitly (maybe it already does and I just missed it?).

If this is a documentation issue rather than a bug, let's definitely save this for a later PR. I'm just unclear at the moment how tonic biases get modified (either in an original Network instance or a copied instance).

I think the current implementation doesn't explicitly have methods for ad hoc modification, though it's certainly possible by modifying the dicts directly, as you point out (and as is done in the evoked example). But both are easy to add, since both drives and biases don't really "materialise" until they're baked into the simulation in NetworkBuilder.

ow are they supposed to do this?

To answer your question: either modify the dict, or create a new network and add the correct bias.

I think the modification thing is a separate discussion to have :) You can always change one line in your script and re-run those lines ... so not even sure if it's a strong use case

What's weird though is that one of the examples shows how to modify the drives ... so it suggests that any other modifications should work. I'm almost inclined to remove the syncing example until we can properly think about how to modify the drives on an existing network. Or keep it with the intention of fixing it properly in a future PR ... along with the dozen other things :)

Okay, I'm cool with leaving this to be discussed in more detail later.

hnn_core/network.py

ntolley · 2021-01-17T22:02:29Z

hnn_core/network_builder.py

-                    cell.add_tonic_input(
-                        **self.net.feed_times['tonic'][_short_name(src_type)])
+                if ('tonic' in self.net.external_biases and
+                        src_type in self.net.external_biases['tonic']):


Curious if there's any instance where src_type is not present? No need to change either way, just wondering if this is a real problem that would crop up.

Oh yeah. That's actually the case most often than not: you are only likely to insert a current clamp onto a single cell type, say, L5_pyramidal, and leave the rest alone.

hnn_core/network_builder.py

hnn_core/params.py

ntolley · 2021-01-17T22:15:04Z

hnn_core/tests/test_drives.py

+    hnn_core_root = op.dirname(hnn_core.__file__)
+    params_fname = op.join(hnn_core_root, 'param', 'default.json')
+    params = read_params(params_fname)
+    net = Network(params, legacy_mode=False)


Should there be a test with legacy mode?

All tests are in 'legacy mode', except this one, as it's testing the new API explicitly.

The most insidious place this is used is in network.py:L591:

if self._legacy_mode: # XXX tests must match HNN GUI output target_populations = self.cellname_list

That's needed to get all the tests against HNN GUI to pass. The code will work fine without, but then there'll be zero chance of prng seeds to match those in GUI.

The net._legacy_mode also skips parameter checking for values extracted from param-files: these can contain all manner of rubbish (notably: drive tstart-values that are >tstop)

ntolley · 2021-01-17T22:34:28Z

examples/plot_simulate_evoked.py

+# each drive.
+
+net_sync = net.copy()
+net_sync.external_drives['evdist1']['dynamics']['sync_within_trial'] = True


Future PR idea, function to sync all external drives automatically?

Sure :) I just don't have an idea of how much this syncing business is actually used?

The main benefit would be teasing apart the contribution of specific inputs. Namely, holding every drive fixed except for one that is allowed to vary.

ntolley · 2021-01-17T22:40:31Z

Sorry for so much delay getting to the review. The internals are extremely clean and delightfully easy to review. Like last time most comments are nitpicks/clarifications that can certainly be held off for followup PR's. Really great work @cjayb, I'm excited to start using this API in my own research.

Could the poor coverage be partly explained by the fact that most tests add drives without actually simulating the network?

@jasmainak you definitely deserve the honors!

rythorpe · 2021-01-17T22:52:11Z

hnn_core/network.py

+    def _reset_drives(self):
+        # reset every time called again, e.g., from dipole.py or in self.copy()
+        for drive_name in self.external_drives.keys():
+            self.external_drives[drive_name]['events'] = list()


Along with Nick's comment, should this function also reset self.external_biases?

I would say there's nothing to reset there. Note that the _reset doesn't actually remove the drives, only the event (spike) times from the appropriate dict key. The external_biases are just a dict of dict, the parameter values in which get applied (HOC) in NetworkBuilder when creating the cells (e.g., current clamp applied soma).

We discussed the possibility of delete-methods for drives (and biases) way back. I don't think there's really a use case for that, do you? On the other hand, it would be as simple as deleting a dictionary key... We can consider it when discussing how seeding should occur in the post-GUI days.

As I mentioned above, let's save this to be discussed in a later PR. I do think that there should be a delete method for drives and biases so that there is both the functionality to add and remove drives/biases from a Network instance (particularly since some of our examples demo the ability to modify attributes of a copied Network instance to run different simulations).

rythorpe · 2021-01-17T23:13:58Z

Pending my one comment above, I haven't been able to break this branch. I'm good to go as soon as it's resolved! This has been a crazy undertaking @cjayb and, like Nick, I can't tell you how excited I am to use the new API!

cjayb · 2021-01-18T09:18:03Z

Thanks all for comments and reviews! I agree it was a mouthful, but between the four of us, Travis and CircleCI, I have a good feeling about the end result :) Time will tell what parts of the API turn out to be annoying...

Could the poor coverage be partly explained by the fact that most tests add drives without actually simulating the network?

I don't know! Honestly, I haven't tried to understand how those values get estimated. Apparently me hitting all the ValueErrors in the new add-methods didn't do a thing to the coverage. This will change once we have a PR that addresses 2. in #233.

I've started a new Issue #244 to discuss tests, which I think needs to be the next step moving forward.

jasmainak · 2021-01-18T21:45:09Z

Incredible, it's in!!! Congrats @cjayb 🥳 🍺 🍻 🚀 🧠

Next steps is to improve the documentation and clean/solidify/test the code for a bit. No more big PRs for a while ;-) Use the new API in your data analysis! Also cc @kohl-carmen

cjayb · 2021-01-18T21:47:13Z

Whoot🥳

jasmainak mentioned this pull request Nov 30, 2020

[MRG] clarify alpha example by removing evoked inputs #213

Merged

jasmainak added this to the 0.1 milestone Nov 30, 2020

jasmainak reviewed Nov 30, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

This was referenced Dec 1, 2020

Nomenclature: 'feeds', 'drives' and 'biases' #222

Closed

MRG ENH: start adding tonic input #209

Merged

jasmainak reviewed Dec 3, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

rythorpe mentioned this pull request Dec 7, 2020

MRG: fix reference to initial prng seedcore params #212

Merged

cjayb force-pushed the refactor_drives_api branch from 552c922 to 80ac507 Compare December 8, 2020 17:21

cjayb commented Dec 8, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

cjayb commented Dec 8, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

cjayb commented Dec 8, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

cjayb commented Dec 8, 2020

View reviewed changes

hnn_core/network.py Show resolved Hide resolved

cjayb commented Dec 8, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

cjayb commented Dec 8, 2020

View reviewed changes

hnn_core/network_builder.py Outdated Show resolved Hide resolved

cjayb commented Dec 8, 2020

View reviewed changes

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

cjayb commented Dec 8, 2020

View reviewed changes

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

jasmainak reviewed Dec 8, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

rythorpe reviewed Dec 8, 2020

View reviewed changes

hnn_core/cell.py Show resolved Hide resolved

rythorpe reviewed Dec 8, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

rythorpe reviewed Dec 8, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

jasmainak reviewed Dec 10, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

jasmainak reviewed Dec 10, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

cjayb commented Dec 13, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

jasmainak reviewed Dec 14, 2020

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

jasmainak reviewed Dec 14, 2020

View reviewed changes

hnn_core/network.py Show resolved Hide resolved

cjayb added 2 commits January 17, 2021 11:31

DOC improve copy doc and add test; update whats new

791303f

DOC make _NetworkDrive private and improve REPR (flake8)

d05c026

cjayb force-pushed the refactor_drives_api branch from 4390052 to d05c026 Compare January 17, 2021 12:35

jasmainak approved these changes Jan 17, 2021

View reviewed changes

ntolley reviewed Jan 17, 2021

View reviewed changes

hnn_core/drives.py Show resolved Hide resolved

ntolley reviewed Jan 17, 2021

View reviewed changes

hnn_core/network.py Outdated Show resolved Hide resolved

ntolley reviewed Jan 17, 2021

View reviewed changes

hnn_core/network.py Show resolved Hide resolved

ntolley reviewed Jan 17, 2021

View reviewed changes

hnn_core/network_builder.py Outdated Show resolved Hide resolved

ntolley reviewed Jan 17, 2021

View reviewed changes

hnn_core/params.py Outdated Show resolved Hide resolved

ntolley reviewed Jan 17, 2021

View reviewed changes

ntolley approved these changes Jan 17, 2021

View reviewed changes

rythorpe reviewed Jan 17, 2021

View reviewed changes

COSMIT code clarity improvements

58efb61

cjayb mentioned this pull request Jan 18, 2021

Moving towards tests independent of legacy GUI output #244

Open

4 tasks

rythorpe approved these changes Jan 18, 2021

View reviewed changes

jasmainak merged commit 84b0fe4 into jonescompneurolab:master Jan 18, 2021

rythorpe mentioned this pull request Jan 20, 2021

handling modifications to or deletion of network drives/biases #248

Closed

jasmainak mentioned this pull request Feb 5, 2021

MPIBackend does not allow passing whole objects #210

Closed

cjayb deleted the refactor_drives_api branch February 17, 2021 23:31

[MRG] Refactor API for creating external network drives #221

[MRG] Refactor API for creating external network drives #221

Conversation

cjayb commented Nov 30, 2020 • edited

jasmainak commented Nov 30, 2020

cjayb commented Dec 8, 2020

jasmainak commented Dec 8, 2020

cjayb commented Dec 8, 2020

cjayb commented Dec 14, 2020

cjayb commented Jan 17, 2021

jasmainak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjayb Jan 18, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmainak Jan 18, 2021 • edited

Choose a reason for hiding this comment

jasmainak Jan 18, 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

ntolley Jan 17, 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

Choose a reason for hiding this comment

ntolley commented Jan 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rythorpe commented Jan 17, 2021

cjayb commented Jan 18, 2021

jasmainak commented Jan 18, 2021

cjayb commented Jan 18, 2021

cjayb commented Nov 30, 2020 •

edited

cjayb Jan 18, 2021 •

edited

jasmainak Jan 18, 2021 •

edited

jasmainak Jan 18, 2021 •

edited

ntolley Jan 17, 2021 •

edited