[MRG] Add ability to record synaptic currents #523

ntolley · 2024-11-20T22:10:33Z

Closes #363 and #533

As discussed with @jnsbck in #515, this PR adds synapse_current_names as an attribute to the base module, and then appends this to the synapse states in _get_state_names(). The end goal is for currents to be recordable via:

net.record('IonotropicSynapse_current')

Some lingering thoughts:

Is it worth changing "current" to "i" for consistency with the channel naming conventions?
It took me awhile to figure out that I needed to modify the View class. Will it be necessary to add a self._synapses_in_view() property for this to work properly?
Add some tests that check basic assumptions of the current recording?

ntolley · 2024-11-20T22:34:29Z

FYI here's the currents and voltages from the tutorial network

Seems reasonable to me? I'll admit I'm just guessing on the units for the synaptic current 😄

jnsbck · 2024-11-21T10:28:51Z

Nice! Just had a superficial look and it looks like not a lot of changes had to be made for the above to work. Units should be the standard current units if I am not mistaken (see here) so nA seems right. Regarding your lingerning thoughts:

I'd also be in favor of changing it to "i"
The View v Module has a small learning curve indeeed! You might not have to add a _synapses_in_view, but we should make sure the membrane currents are filtered by the synapses in view. sth like [i for i, syn in zip(self.base.synapse_current_names, self.synapses) if syn._name in i] should do the trick. Also for more intuition on View v Module see jaxley api tutorial, (I assume you already read the docstrings :) )
Yes, we should definitely make sure this is tested well. (which I just noticed we don't do for channel_currents at the moment), maybe you can add a quick test for i_HH along with the synapse current to

jaxley/tests/test_record_and_stimulate.py

Line 57 in 5e3637c

def test_record_synaptic_and_membrane_states(SimpleNet):

other than testing that it works, I don't know what would be sensible to check though. @michaeldeistler any ideas?

Lemme me know when you want a review :)

michaeldeistler · 2024-11-22T07:17:02Z

Awesome, thanks a lot for getting this started @ntolley!

I think the following would be important to test:

It plays nicely with advanced indexing of synapses. E.g., to record only synaptic currents of for pre-synaptic neuron 3 and 4:

fully_connect(net.cell("all"), net.cell("all"), IonotropicSynapse())
net.copy_node_property_to_edges("global_cell_index")
df = net.edges
df = df.query("pre_global_cell_index in [3, 4]")
net.select(edges=df.index).record("Ionotropic_s")

If there are multiple kinds of synapses (e.g. IonotropicSynapse and TanhSynapse), then there is no mixup between them. E.g.:

fully_connect(net.cell("all"), net.cell("all"), IonotropicSynapse())
fully_connect(net.cell("all"), net.cell("all"), TanhSynapse())
df = net.edges
df = df.query("SynapseType == 'IonotropicSynapse'")
net.select(edges=df.index).record("Ionotropic_s")

Let me know if anything is unclear!

ntolley · 2024-11-26T18:38:27Z

jaxley/modules/base.py

@@ -2422,7 +2426,8 @@ def __init__(
        )

        self.channels = self._channels_in_view(pointer)
-        self.membrane_current_names = [c._name for c in self.channels]
+        self.membrane_current_names = [c.current_name for c in self.channels]


Think this was a bug, it wasn't actually possible to record channels currents from a View because of the naming

good catch!

ntolley · 2024-11-26T18:53:16Z

Got sent down a rabbit hole that led to #533, unfortunately edges aren't being updated in the View so that's going to need to be fixed to actually have the correct synpases be filtered out.

ntolley · 2024-11-26T19:12:08Z

jaxley/modules/base.py

@@ -2490,7 +2495,7 @@ def _set_inds_in_view(
            ].unique()
            pre = base_edges["pre_global_comp_index"].isin(incl_comps).to_numpy()
            post = base_edges["post_global_comp_index"].isin(incl_comps).to_numpy()
-            possible_edges_in_view = base_edges.index.to_numpy()[(pre & post).flatten()]
+            possible_edges_in_view = base_edges.index.to_numpy()[(pre | post).flatten()]


@michaeldeistler @jnsbck this is the culprit for #533 ! Essentially if you're indexing net.cell(0), then it's impossible for a view of that cell to include the compartments of another cell (unless it's an autapse)

I think this is a question of how you want this to behave. I've changed it to an "or" condition, so net.cell(0).edges will include both synapses onto this cell, as well as synapses it makes with other cells

For the purposes of this PR, it would make sense to only include the edges of the post synaptic cell, however you could argue for different expected behaviors.

I decided to just go forward with only appending the post synaptic target so that I could finish the tasks for this PR. We can discuss if that's actually the desired behavior for the View

…aptic cell

ntolley · 2024-11-26T22:59:54Z

jaxley/modules/base.py

@@ -1207,9 +1208,14 @@ def _get_state_names(self) -> Tuple[List, List]:

        Returns states seperated by comps and edges."""
        channel_states = [name for c in self.channels for name in c.channel_states]
-        synapse_states = [name for s in self.synapses for name in s.synapse_states]
+        synapse_states = [
+            name for s in self.synapses if s is not None for name in s.synapse_states


the not None filter is necessary since self.synapses is populated with [None] under certain conditions

I'll admit I don't totally understand why it's necessary just yet, although I saw the comment about it preserving indexing 😄

ntolley · 2024-11-26T23:14:10Z

I think I've fixed most of the issues! The new tests are passing so this is ready for a review

michaeldeistler

This is awesome, thanks a ton!

A few comments below, mostly minor things. I suggest we stick to pre & post for now, we could change this behavior in another PR if we want to. Also, please add an entry in the changelog describing the new feature and the bug fixes :)

Good to go once changes are addressed! Thanks again!

michaeldeistler · 2024-11-27T07:54:06Z

jaxley/modules/base.py

@@ -2422,7 +2426,8 @@ def __init__(
        )

        self.channels = self._channels_in_view(pointer)
-        self.membrane_current_names = [c._name for c in self.channels]
+        self.membrane_current_names = [c.current_name for c in self.channels]


good catch!

michaeldeistler · 2024-11-27T07:55:33Z

jaxley/modules/base.py

            post = base_edges["post_global_comp_index"].isin(incl_comps).to_numpy()
-            possible_edges_in_view = base_edges.index.to_numpy()[(pre & post).flatten()]
+            possible_edges_in_view = base_edges.index.to_numpy()[(post).flatten()]


let's revert this to pre & post, see my response in #533

michaeldeistler · 2024-11-27T07:56:56Z

tests/test_record_and_stimulate.py

    net.cell(2).branch(0).loc(0.0).record("v")
    net.IonotropicSynapse.edge(1).record("IonotropicSynapse_s")
    net.cell(2).branch(0).loc(0.0).record("HH_m")
    net.cell(1).branch(0).loc(0.0).record("v")
    net.TestSynapse.edge(0).record("TestSynapse_c")
    net.cell(1).branch(0).loc(0.0).record("HH_m")
+    net.cell(1).branch(0).loc(0.0).record("i_HH")


ntolley · 2024-11-27T20:52:02Z

Think all the comments have been addressed, thanks @michaeldeistler @jnsbck for the help with this PR 🙌

michaeldeistler

LGTM, thanks a lot!

jnsbck · 2024-12-02T08:42:05Z

Awesome! Thanks for tackling this! 🚀

ntolley commented Nov 26, 2024

View reviewed changes

ntolley added 4 commits November 26, 2024 13:41

Add test for recording currents

90da612

Add synapse current attribute for recordings

ab80c0d

black formatting

791fd91

Fix channel current recording and add tests

94ea4a3

ntolley force-pushed the synapse_currents branch from 3e8722d to 94ea4a3 Compare November 26, 2024 18:41

ntolley mentioned this pull request Nov 26, 2024

BUG: Edge properties not accessible in View #533

Closed

possible fix to edge dataframe

4d7e291

ntolley commented Nov 26, 2024

View reviewed changes

Add tests for advanced synapse indexing, only store edges of post syn…

dbd072e

…aptic cell

ntolley commented Nov 26, 2024

View reviewed changes

ntolley changed the title ~~WIP: Add ability to record synaptic currents~~ [MRG] Add ability to record synaptic currents Nov 26, 2024

michaeldeistler approved these changes Nov 27, 2024

View reviewed changes

update changelog, revert edge view filtering

17fbe13

michaeldeistler approved these changes Nov 28, 2024

View reviewed changes

michaeldeistler merged commit d89fe7b into jaxleyverse:main Nov 28, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Add ability to record synaptic currents #523

[MRG] Add ability to record synaptic currents #523

ntolley commented Nov 20, 2024 •

edited

Loading

ntolley commented Nov 20, 2024

jnsbck commented Nov 21, 2024

michaeldeistler commented Nov 22, 2024

ntolley Nov 26, 2024

michaeldeistler Nov 27, 2024

ntolley commented Nov 26, 2024

ntolley Nov 26, 2024

ntolley Nov 26, 2024

ntolley Nov 26, 2024

ntolley commented Nov 26, 2024

michaeldeistler left a comment

michaeldeistler Nov 27, 2024

michaeldeistler Nov 27, 2024

michaeldeistler Nov 27, 2024

ntolley commented Nov 27, 2024

michaeldeistler left a comment

jnsbck commented Dec 2, 2024

[MRG] Add ability to record synaptic currents #523

[MRG] Add ability to record synaptic currents #523

Conversation

ntolley commented Nov 20, 2024 • edited Loading

ntolley commented Nov 20, 2024

jnsbck commented Nov 21, 2024

michaeldeistler commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntolley commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntolley commented Nov 26, 2024

michaeldeistler left a 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 Nov 27, 2024

michaeldeistler left a comment

Choose a reason for hiding this comment

jnsbck commented Dec 2, 2024

ntolley commented Nov 20, 2024 •

edited

Loading