[MRG] Add ability to record voltages and currents from all sections #502
Conversation
hnn_core/cell.py
Outdated
| @@ -548,6 +550,39 @@ def record_soma(self, record_vsoma=False, record_isoma=False): | |||
| if record_vsoma: | |||
| self.rec_v.record(self._nrn_sections['soma'](0.5)._ref_v) | |||
|
|
|||
| def record_sec(self, record_vsec=False, record_isec=False): | |||
There was a problem hiding this comment.
soma is also a section in the Neuron scheme of things. Would it make sense to unify the API with the recording of soma?
There was a problem hiding this comment.
For sure! While I'm still testing things I want to keep the code separate until I know recordings work for all sections. Then I can delete all the soma specific code (and set an option between recording just the soma, or all sections).
ntolley
changed the title
Codecov Report
@@ Coverage Diff @@
## master #502 +/- ##
==========================================
- Coverage 91.81% 91.77% -0.05%
==========================================
Files 22 22
Lines 4214 4229 +15
==========================================
+ Hits 3869 3881 +12
- Misses 345 348 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@rythorpe @jasmainak took a little sabbatical from this PR but everything seems to be working now! Might be a nice add-on to the 0.3 😉 |
doc/whats_new.rst
Outdated
| @@ -85,6 +88,12 @@ API | |||
| :meth:`~hnn_core.extracellular.ExtracellularArray.plot_lfp`, by | |||
| `Steven Brandt`_ and `Ryan Thorpe`_ in :gh:`517`. | |||
|
|
|||
| - Recorded voltages/currents from the soma, as well all sections, are enabled by | |||
| setting either `record_vsoma` and `record_isoma`, or `record_vsec` and `record_isec` | |||
There was a problem hiding this comment.
As I also suggested below, why do we need record_vsoma and record_isoma?
There was a problem hiding this comment.
Thought process here is
- Consistency with old API (although I'm renaming the attributes of
net.cell_responsehere so perhaps it's a moot point) - Have to imagine there's going to be a memory issue with larger networks or some computational burden when storing all this data. Could be nice to not store all currents/voltages if you only care about those in the soma
hnn_core/cell.py
Outdated
| Option to record somatic currents from cells | ||
|
|
||
| Option to record somatic currents from cells. Default: False. | ||
| record_vsec : bool |
There was a problem hiding this comment.
voltage_secs : 'soma' | 'all' | list
? But you decide, I don't want to add unnecessary complexity
There was a problem hiding this comment.
What do you think of record_vsec : 'soma' | 'all' | None?
I'm all for it, but if we propagate this change to simulate_dipole() it'd be an API change. I don't know of anyone (besides me) using this functionality in hnn-core so a deprecation warning seems unnecessary, but worth bringing up here.
There was a problem hiding this comment.
I think it's okay to make a breaking change here ... we'll at least know who are using hnn-core for real if they complain :P
There was a problem hiding this comment.
Just pushed the change! I think the code definitely reads better without lugging around 4 parameters everywhere
|
Cursorily looks fine barring the tiny comment. I trust you tested it properly :) |
hnn_core/cell.py
Outdated
| Contains recording of somatic currents indexed | ||
| by synapse type. (keys are soma_gabaa, soma_gabab etc.) | ||
| Must be enabled by running simulate_dipole(net, record_isoma=True) | ||
| rec_vsec : dict |
There was a problem hiding this comment.
should we just call it vsec to avoid confusion with record_vsec which is the boolean that decides whether to record vsec or not?
There was a problem hiding this comment.
Sounds good! Will be ready to merge once I push this change
|
good to go from your end @ntolley ? |
All good with me once the CI's are green! Just pushed the last commit addressing your comment |
|
Thanks @ntolley ! great stuff 🥳 🥳 🥳 |
This is an enhancement I've been meaning to add for awhile. Not sure if this is going to cause a lot of problems in terms of data transfer, but we'll see!
The way things are currently written are very redundant with the somatic recording functionality. Once I know this is working, the
record_vsomaandrecord_isomaoptions tosimulate_dipolecan use the same code as here by restricting the section names considered to only be the'soma'.