[MRG] add method for updating cell biophysics from Network instance

[rythorpe]

closes #316

The goal here is to create an API for users to adjust biphysics and synapse properties as they do in HNN-GUI.

[codecov-commenter]

Codecov Report

Merging #321 (66dda85) into master (32d33cc) will decrease coverage by 0.06%.
The diff coverage is 98.64%.

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

@@            Coverage Diff             @@
##           master     #321      +/-   ##
==========================================
- Coverage   90.43%   90.36%   -0.07%     
==========================================
  Files          13       13              
  Lines        2415     2439      +24     
==========================================
+ Hits         2184     2204      +20     
- Misses        231      235       +4     

Impacted Files	Coverage Δ
hnn_core/drives.py	92.25% <ø> (ø)
hnn_core/network.py	91.95% <96.87%> (-0.53%)	⬇️
hnn_core/cell.py	97.25% <100.00%> (+0.09%)	⬆️
hnn_core/cell_response.py	83.73% <100.00%> (ø)
hnn_core/cells_default.py	97.75% <100.00%> (-0.08%)	⬇️
hnn_core/network_builder.py	92.16% <100.00%> (-0.06%)	⬇️
hnn_core/viz.py	87.50% <100.00%> (ø)

---

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 32d33cc...e9c79bf. Read the comment docs.

[cjayb]

@rythorpe could you narrow down the scope of this PR? Is you intention to create an API for modifying the values of the existing/implemented biophysical parameters? In contrast to implementing new biophysics (e.g. calcium)?

[rythorpe]

@rythorpe could you narrow down the scope of this PR? Is you intention to create an API for modifying the values of the existing/implemented biophysical parameters? In contrast to implementing new biophysics (e.g. calcium)?

@cjayb This is just for modifying the values of previously existing biophysics/synapse parameters.

[cjayb]

Perfect!

[rythorpe]

This PR still needs some work, but I'd like some feedback before moving forward @jasmainak @cjayb.

I initially tried instantiating the population of Cell objects within Network and then building them in network_builder, but that was probably overkill for this PR and, more importantly, caused pickling errors. It'd be nice to have Cell objects accessible to the user, but idk if it's easily implementable or needed for most use cases. @cjayb would this be desirable for any of your work with LFP calculations?

[jasmainak]

There shouldn't be pickling errors anymore. I think for the calcium dynamics ... see #333 ideal API would be:

from hnn_core.default_cells import pyramidal_ca

net.cells['L5Pyr'] = pyramidal_ca()

You should first start by moving p_secs, topology etc as an attribute of Cell class. Check if the cell remains picklable once you do that (there is a test for that). Once you are able to do that, it's "just" a matter of exposing it in the Network class ... I'd really like us to go this way so the Cell class can evolve to have methods that are accessible to the user.

For example, you can imagine a method insert_mechanism that updates self.p_secs. That way, users are free to try new mechanisms, add new synapses etc., and not just modify the existing cell.

[rythorpe]

@jasmainak are you imagining that all the cells will be accessible from Network, or just a template cell that gets replicated downstream by NetworkBuilder? We had originally discussed the latter, but the former might be nice as well.

[jasmainak]

template cell for now I would say. Making all the cells accessible might be helpful if we were to combine Cell and CellResponse class. See also #217 . Then the Cell class will begin to become a real workhorse, but we're not there yet.

[ntolley]

Agreed with putting off making them all accessible until later. I really didn't see any immediate benefits while working to convert CellResponse to a list, the current data structure is pretty straightforward for spikes, voltages etc. If it simplifies things to merge CellResponse and Cell that's definitely great.

Sadly that old PR is more or less just moving things around, but if there's a compelling argument to push it forward I'm all ears.

[rythorpe]

I'm just finishing up running the tests, but I've left access to Cell objects from Network intact for simplicity. If we decide we'd rather wait to include this in master, it's pretty easy to migrate Cell population instantiation into NetworkBuilder.

[rythorpe]

Here's what the API looks like for changing a biophysical synapse parameter. Note that it also produces the anticipated results before vs. after manual adjustment of tau2:

[jasmainak]

yeaahhhh! this is the right direction to go :) Could we hide update_cells from the user? They shouldn't have to worry about it. Should be called during the build somewhere

[rythorpe]

Oh good point, I think I can throw it in NetworkBuilder._create_cells_and_drives().

[jasmainak]

I didn't review properly but looks pretty clean on a superficial glance. @ntolley you want to take a first stab at the review?

[rythorpe]

@jasmainak I tried running

import os.path as op
from mne.utils import object_size
import hnn_core
from hnn_core import read_params, Network

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)

size = object_size(net) / (1024 * 1024)

but get a runtime error reporting that type Network is unsupported. Am I doing this right?

[jasmainak]

Am I doing this right?

humm ... indeed. I get this too. MNE probably supports only certain known datatypes etc for this. Anyhow, I deleted my comment because I realized that the cell_type_names list is only containing 4 items (right?), so it shouldn't bloat things too much. You could probably just measure sizeof(cell_type_names) for curiosity if you want

[jasmainak]

I was brave and went ahead and merged the PR. Thanks for the great effort @rythorpe !

What's the next PR? :)

[rythorpe]

humm ... indeed. I get this too. MNE probably supports only certain known datatypes etc for this. Anyhow, I deleted my comment because I realized that the cell_type_names list is only containing 4 items (right?), so it shouldn't bloat things too much. You could probably just measure sizeof(cell_type_names) for curiosity if you want

Oh I see. I do want to be conscious of how big Network gets though because it can bog things down. Hopefully the recent changes keep it slim....

[rythorpe]

I was brave and went ahead and merged the PR. Thanks for the great effort @rythorpe !

What's the next PR? :)

Woooo! Thanks for the reviews! Have we revitalized the updated calcium dynamics PR?

[jasmainak]

Take a look at #333 to see what's left to do. We need to update the examples to make them look like before. How are we going to do the ground-truth tests once calcium dynamics come? Do we do #314 first to update the ground truth?

[jasmainak]

I do want to be conscious of how big Network gets though because it can bog things down.

we probably want to deprecate net._params further down the line so there is no duplication of parameters through the Cell object. It's also cleaner structurally and code-wise.