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] Refactor creation of network 'feeds' (to allow new mechanism for adding network drives) #191

Merged
merged 9 commits into from Nov 16, 2020
Merged

[MRG] Refactor creation of network 'feeds' (to allow new mechanism for adding network drives) #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
861129a
moved gid_dict update to Network, MAINT docs
cjayb Sep 26, 2020
fffc009
refactor feed instantiation to Network class
cjayb Nov 5, 2020
6a0be7f
make n_trials explicit in instantiate_feeds
cjayb Nov 16, 2020
d36e36a
update whats new
cjayb Nov 16, 2020
da134c7
move times to Spikes and simplify
cjayb Nov 16, 2020
6646a56
fix error in test
cjayb Nov 16, 2020
109ea9b
raise a proper TypeError
cjayb Nov 16, 2020
537e0c4
simplify gid initialisation in _Cell and _A.Cell
cjayb Nov 16, 2020
7d16670
better proposal for cell gid checking, with test
cjayb Nov 16, 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
2 changes: 2 additions & 0 deletions doc/whats_new.rst
View file
Open in desktop
Expand Up @@ -33,6 +33,8 @@ Changelog

- Add ability to record somatic voltages from all cells, by `Nick Tolley`_ in `#190 <https://github.com/jonescompneurolab/hnn-core/pull/190>`_

- Add ability to instantiate external feed event times of a network prior to building it, by `Christopher Bailey`_ in `#191 <https://github.com/jonescompneurolab/hnn-core/pull/191>`_

Bug
~~~

Expand Down
15 changes: 7 additions & 8 deletions examples/plot_firing_pattern.py
View file
Open in desktop
Expand Up @@ -34,8 +34,8 @@

###############################################################################
# The cell IDs (gids) are stored in the network object as a dictionary
gid_dict = net.gid_dict
print(net.gid_dict)
gid_ranges = net.gid_ranges
print(net.gid_ranges)

###############################################################################
# Simulated voltage in the soma is stored in the Spikes object as a dictionary.
Expand All @@ -46,9 +46,8 @@
###############################################################################
# We can plot the firing pattern of individual cells by indexing with the gid
gid = 170
times = net.spikes.times[trial_idx]
plt.figure(figsize=(4, 4))
plt.plot(times, vsoma[gid])
plt.plot(net.spikes.times, vsoma[gid])
plt.title('%s (gid=%d)' % (net.gid_to_type(gid), gid))
plt.xlabel('Time (ms)')
plt.ylabel('Voltage (mV)')
Expand All @@ -61,9 +60,9 @@
fig, axes = plt.subplots(3, 1, figsize=(5, 7), sharex=True)

for idx in range(10): # only 10 cells per cell-type
gid = gid_dict['L2_pyramidal'][idx]
axes[0].plot(times, vsoma[gid], color='g')
gid = gid_dict['L5_pyramidal'][idx]
axes[0].plot(times, vsoma[gid], color='r')
gid = gid_ranges['L2_pyramidal'][idx]
axes[0].plot(net.spikes.times, vsoma[gid], color='g')
gid = gid_ranges['L5_pyramidal'][idx]
axes[0].plot(net.spikes.times, vsoma[gid], color='r')
net.spikes.plot(ax=axes[1])
net.spikes.plot_hist(ax=axes[2], spike_types=['L5_pyramidal', 'L2_pyramidal'])
4 changes: 2 additions & 2 deletions examples/plot_simulate_evoked.py
View file
Open in desktop
Expand Up @@ -69,9 +69,9 @@
###############################################################################
# We can additionally calculate the mean spike rates for each cell class by
# specifying a time window with tstart and tstop.
all_rates = spikes.mean_rates(tstart=0, tstop=170, gid_dict=net.gid_dict,
all_rates = spikes.mean_rates(tstart=0, tstop=170, gid_ranges=net.gid_ranges,
mean_type='all')
trial_rates = spikes.mean_rates(tstart=0, tstop=170, gid_dict=net.gid_dict,
trial_rates = spikes.mean_rates(tstart=0, tstop=170, gid_ranges=net.gid_ranges,
mean_type='trial')
print('Mean spike rates across trials:')
print(all_rates)
Expand Down
36 changes: 28 additions & 8 deletions hnn_core/basket.py
View file
Open in desktop
Expand Up @@ -21,9 +21,9 @@ class BasketSingle(_Cell):
names of section locations that are proximal or distal.
"""

def __init__(self, gid, pos, cell_name='Basket'):
def __init__(self, pos, cell_name='Basket', gid=None):
self.props = self._get_soma_props(cell_name, pos)
_Cell.__init__(self, gid, self.props)
_Cell.__init__(self, self.props, gid=gid)
# store cell name for later
self.name = cell_name

Expand Down Expand Up @@ -62,12 +62,22 @@ def _synapse_create(self):


class L2Basket(BasketSingle):
"""Class for layer 2 basket cells."""
"""Class for layer 2 basket cells.

def __init__(self, gid=-1, pos=-1):
Parameters
----------
pos : tuple
Coordinates of cell soma in xyz-space
gid : int or None (optional)
Each cell in a network is uniquely identified by it's "global ID": GID.
The GID is an integer from 0 to n_cells, or None if the cell is not
yet attached to a network. Once the GID is set, it cannot be changed.
"""

def __init__(self, pos, gid=None):
# BasketSingle.__init__(self, pos, L, diam, Ra, cm)
# Note: Basket cell properties set in BasketSingle())
BasketSingle.__init__(self, gid, pos, 'L2Basket')
BasketSingle.__init__(self, pos, cell_name='L2Basket', gid=gid)
self.celltype = 'L2_basket'

self._synapse_create()
Expand All @@ -76,11 +86,21 @@ def __init__(self, gid=-1, pos=-1):


class L5Basket(BasketSingle):
"""Class for layer 5 basket cells."""
"""Class for layer 5 basket cells.

Parameters
----------
pos : tuple
Coordinates of cell soma in xyz-space
gid : int or None (optional)
Each cell in a network is uniquely identified by it's "global ID": GID.
The GID is an integer from 0 to n_cells, or None if the cell is not
yet attached to a network. Once the GID is set, it cannot be changed.
"""

def __init__(self, gid=-1, pos=-1):
def __init__(self, pos, gid=None):
# Note: Cell properties are set in BasketSingle()
BasketSingle.__init__(self, gid, pos, 'L5Basket')
BasketSingle.__init__(self, pos, cell_name='L5Basket', gid=gid)
self.celltype = 'L5_basket'

self._synapse_create()
Expand Down
52 changes: 47 additions & 5 deletions hnn_core/cell.py
View file
Open in desktop
Expand Up @@ -22,6 +22,10 @@ class _ArtificialCell:
associated with a unique gid).
threshold : float
Membrane potential threshold that demarks a spike.
gid : int or None (optional)
Each cell in a network is uniquely identified by it's "global ID": GID.
The GID is an integer from 0 to n_cells, or None if the cell is not
yet attached to a network. Once the GID is set, it cannot be changed.

Attributes
----------
Expand All @@ -33,8 +37,10 @@ class _ArtificialCell:
nrn_netcon : instance of h.NetCon()
NEURON h.NetCon() object that creates the spike
source-to-target references for nrn_vecstim.
gid : int
GID of the cell in a network (or None if not yet assigned)
"""
def __init__(self, event_times, threshold):
def __init__(self, event_times, threshold, gid=None):
# Convert event times into nrn vector
self.nrn_eventvec = h.Vector()
self.nrn_eventvec.from_python(event_times)
Expand All @@ -47,17 +53,36 @@ def __init__(self, event_times, threshold):
self.nrn_netcon = h.NetCon(self.nrn_vecstim, None)
self.nrn_netcon.threshold = threshold

self._gid = None
if gid is not None:
self.gid = gid # use setter method to check input argument gid

@property
def gid(self):
return self._gid

@gid.setter
def gid(self, gid):
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a realistic usecase for being able to update cell gids? Otherwise, I would just hide the functionality. The less you expose, the better

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well this is a little stronger than hiding, no? I'm not sure the Cell gid ever needs to be updated, certainly not by users! Once it gets set, it should raise all hell if anything tries to change it. However, it would be nice to be able to get the value using cell.gid, hence the @property. I know this complicates the _Cell class a little, but may make reading downstream code clearer (see network_builder:L431-440)?

Copy link
Contributor

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 why we need 2 gid attributes here (i.e., _assigned_gid and gid). Why can't we just have one attribute named _ArtificialCell._gid that is set to the arg gid using your decorated setter function, and is retrieved by your decorated property function? Unless I'm missing something nuanced in the logic, such a change would simplify the gid code in this class as well as subsequent cell classes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, maybe a little convoluted there yes. I just pushing something now, which I instantly regretted :( Ignore first push, my proposal will be the second one! To answer your question: I'm proposing to protect .gid because we don't want to risk anything messing with a Cell'a gid once building has begun. Perhaps there's a better way of achieving this?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm imagining is something like

def __init__(self, event_times, threshold, gid=None):
   ...
   self.gid = gid

@gid.setter
def gid(self, gid)
   if self.gid is None:
      self.gid = gid
   else:
      raise RuntimeError('Global ID for this cell already assigned!')

Copy link
Collaborator Author

@cjayb cjayb Nov 16, 2020 •
edited

Choose a reason for hiding this comment

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

Same here with the browser refresh ;) I'm trying your proposal, but test_cell hangs? I would be surprised if you could do self.gid = gid, isn't that trying to call the 'getter' and setter at the same time?! I've seen the _-prefix in cases like this elsewhere, thought it was a 'pattern'...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but the only consideration here is that the gid of a given cell should only be assigned to a value other than None once.

That's my goal, yes.

Copy link
Collaborator

@jasmainak jasmainak Nov 16, 2020 •
edited

Choose a reason for hiding this comment

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

let me shed some light here. You can use private attributes (starting with underscore) for stuff that you don't want the user to mess with. For example if the object is complex and updating one attribute actually affects the rest of the object. For instance if the object had an attribute called sfreq (sampling frequency) and another attribute called times, you would have to update both simultaneously. This is why it sometimes makes sense to protect certain attributes and update them only through special functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here with the browser refresh ;) I'm trying your proposal, but test_cell hangs? I would be surprised if you could do self.gid = gid, isn't that trying to call the 'getter' and setter at the same time?!

Shoot, you're right. I think your implementation in 7d16670 is our best bet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can always be changed to something smoother later.

if not isinstance(gid, int):
raise ValueError('gid must be an integer')
if self._gid is None:
self._gid = gid
else:
raise RuntimeError('Global ID for this cell already assigned!')


class _Cell(ABC):
"""Create a cell object.

Parameters
----------
gid : int
The cell ID
soma_props : dict
The properties of the soma. Must contain
keys 'L', 'diam', and 'pos'
gid : int or None (optional)
Each cell in a network is uniquely identified by it's "global ID": GID.
The GID is an integer from 0 to n_cells, or None if the cell is not
yet attached to a network. Once the GID is set, it cannot be changed.

Attributes
----------
Expand All @@ -70,15 +95,19 @@ class _Cell(ABC):
rec_v : h.Vector()
Recording of somatic voltage. Must be enabled
by running simulate_dipole(net, record_vsoma=True)
gid : int
GID of the cell in a network (or None if not yet assigned)
"""

def __init__(self, gid, soma_props):
self.gid = gid
def __init__(self, soma_props, gid=None):
# variable for the list_IClamp
self.list_IClamp = None
self.soma_props = soma_props
self.create_soma()
self.rec_v = h.Vector()
self._gid = None
if gid is not None:
self.gid = gid # use setter method to check input argument gid

def __repr__(self):
class_name = self.__class__.__name__
Expand All @@ -88,6 +117,19 @@ def __repr__(self):
soma_props['Ra'], soma_props['cm']))
return '<%s | %s>' % (class_name, s)

@property
def gid(self):
return self._gid

@gid.setter
def gid(self, gid):
if not isinstance(gid, int):
raise ValueError('gid must be an integer')
if self._gid is None:
self._gid = gid
else:
raise RuntimeError('Global ID for this cell already assigned!')

@abstractmethod
def get_sections(self):
"""Get sections in a cell."""
Expand Down
2 changes: 2 additions & 0 deletions hnn_core/dipole.py
View file
Open in desktop
Expand Up @@ -44,6 +44,8 @@ def simulate_dipole(net, n_trials=None, record_vsoma=False):

if n_trials is not None:
net.params['N_trials'] = n_trials
# need to redo these if n_trials changed after net.__init__()!
net._instantiate_feeds(n_trials=n_trials)
else:
n_trials = net.params['N_trials']

Expand Down
5 changes: 1 addition & 4 deletions hnn_core/mpi_child.py
View file
Open in desktop
Expand Up @@ -100,13 +100,10 @@ def run(self, params):
from hnn_core import Network
from hnn_core.parallel_backends import _clone_and_simulate

prng_seedcore_initial = params['prng_*']

net = Network(params)
sim_data = []
for trial_idx in range(params['N_trials']):
single_sim_data = _clone_and_simulate(net, trial_idx,
prng_seedcore_initial)
single_sim_data = _clone_and_simulate(net, trial_idx)

# go ahead and append trial data for each rank, though
# only rank 0 has data that should be sent back to MPIBackend
Expand Down