[MRG] feat: Extracting Katharina's voltage-initialization changes #1185

asoplata · 2025-11-06T21:56:16Z

This is the first commit of many to come where I'm going to extract the parts of #1168 relevant to the new, proper voltage initialization.

The plan is:

We'll separate the changes to the voltage initialization stuff (and nothing else) and make it its own PR onto master (this PR),
Update the tests that become broken as we go along,
After all the updates are applied, then everyone reviews the changes (which will likely end up in a slightly different form from how they currently exist in [WIP] Experimental duecker_ET model branch #1168),
Then we merge these voltage initialization changes into master,
Then finally, we merge/rebase the voltage initialization changes (which are now on master) back into the code at [WIP] Experimental duecker_ET model branch #1168 .
Repeat the process for all other code features included in the lengthy [WIP] Experimental duecker_ET model branch #1168 work, until everything gets merged, piece-by-piece.

For starters, this first commit consists of only the extracted changes to cell.py and nothing else. Fortunately, in this case, the tests seem to pass without having to change anything.

asoplata · 2025-11-07T14:35:45Z

I think this is ready for review and merge as-is. Note that as far as I can tell, this does NOT change the initial conditions of any simulation, and output appears to be identical to how it was before. (You can check this with one of the final tests, which is pytest hnn_core/tests/test_parallel_backends.py::TestParallelBackends::test_compare_hnn_core, and directly compares simulation output to saved dipole data.)

I was planning on writing tests for the new voltage-initialization, but as it turns out we have very, very little testing of cells_default.py in total. Many of the docstrings in cells_default.py are also extremely out of date, limited, or incorrect.

However, since #1168 includes a large number of changes to cells_default.py which are independent of voltage-initialization, I think it's better to defer expansion of testing and documenting for that file to a subsequent PR. There will be soon/eventually be a subsequent PR that introduces the main Duecker_ET model itself, and that will include all the changes in cells_default.py relevant to that model. I think it makes more sense to re-evaluate the testing of the file at that time, since there may be some opportunities for refactoring that make writing the tests easier (or not).

asoplata · 2025-11-07T14:36:39Z

This will resolve #996 .

asoplata · 2025-11-07T14:39:01Z

Force-pushed in order to remove an incorrect statement in an earlier commit message. (I previously thought that this would change the initial membrane voltages, but I see Katharina went to pains to not do that, and it's correct that the initial membrane voltages for each section are now still the same as they were under the old code).

@katduecker

This is the first commit of many to come where I'm going to extract the parts of jonescompneurolab#1168 relevant to the new, proper voltage initialization. The plan is: 1. We'll separate the changes to the "v_init" stuff and make it its own PR onto `master`, 2. Change the tests that become broken as we go along, 3. Everyone reviews the changes (which will likely end up in a slightly different form from how they currently exist in jonescompneurolab#1168), 4. We merge these voltage initialization changes into `master`, 5. Then finally, we merge/rebase the voltage initialization changes (which are now on `master`) *back* into the code at jonescompneurolab#1168 . 6. Repeat the process for all code features included in the length jonescompneurolab#1168 work, until everything gets merged, piece-by-piece. For starters, this commit consists of *only* the extracted changes to `cell.py` and nothing else. Fortunately, in this case, the tests seem to pass without having to change anything. The original person who actually wrote this code was @katduecker, which is why they are indicated as author. This version of the code that was extracted from jonescompneurolab#1168 was done so by @asoplata .

@katduecker

This does NOT pass tests The original person who actually wrote this code was @katduecker, which is why they are indicated as author. This version of the code that was extracted from jonescompneurolab#1168 was done so by @asoplata .

@katduecker

This also includes regenerated networks, since doing that is necessary to allow the tests to pass as well. The original person who actually wrote this code was @katduecker, which is why they are indicated as author. This version of the code that was extracted from jonescompneurolab#1168 was done so by @asoplata .

@katduecker

This fixes what I think is a bug currently present in jonescompneurolab#1168, where the L2's `soma` section did not have its universal voltage-initialization value applied (but the dendrites did). Additionally, @katduecker , I think the same bug is present in your `_get_interneuron_soma()` function (which I've removed in this commit since it's not relevant to the voltage-initialization). Your `_get_interneuron_soma` takes a `v_init` value, but does not apply it in `Section` creation. This was probably missed. This also removes `NetworkBuilder.state_init()` like KD did, since it is no longer needed, as the voltage initialization is completely moved over to `cells_default.py` now. With this, I think the voltage-initialization stuff has been completely extracted from jonescompneurolab#1168. The only remaining work is to write new tests specifically for the `v_init` where it is used.

asoplata · 2025-11-07T14:47:18Z

Another force-push in order to label @katduecker as the author of most of the commits, since she was the one who wrote the code originally (and this way she gets explicit credit). Is this okay with you Katharina?

No more force-pushes should be necessary.

I believe this adds the last of the code that is extracted from jonescompneurolab#1168 that is *not* the actual Duecker model itself, but instead code changes that are used by the default model. These functions are not currently tested at all, and I have not added tests for them for similar reasons to jonescompneurolab#1185 (comment) After this, we should be able to make a PR that adds the actual Duecker model itself. However, there will be additional work for that step: we need to update `hnn_io.read_network_configuration` and the equivalent `...write...` such that they support the Duecker model. Continuing the pattern, this is based on top of jonescompneurolab#1187.

katduecker · 2025-11-12T19:20:06Z

hnn_core/cell.py

        section_data["Ra"] = self.Ra
        section_data["end_pts"] = self.end_pts
        section_data["nseg"] = self.nseg
+        section_data["v"] = self._v


Not that it matters but should this be self._v?

Was this updated? seems like this should be resolved

katduecker · 2025-11-12T19:21:50Z

hnn_core/cells_default.py



-def _get_pyr_soma(p_all, cell_type):
+# KD: In the new model, the basal dendrites are differently tuned from the apical


My fault , but should it read "Tuning of the basal dendrites in duecker_ET_model is different from apical dendrites"?

katduecker · 2025-11-12T19:25:30Z

hnn_core/cells_default.py

        assert isinstance(override_params, dict)
        p_all = compare_dictionaries(p_all, override_params)

+    all_v_init = -71.46


Does an extra variable add anything here?

maybe just to type the number once since it's reused below?

katduecker · 2025-11-12T19:28:47Z

@asoplata thanks Austin looks great to me, and thanks for giving me the credits for it!

I believe this adds the last of the code that is extracted from jonescompneurolab#1168 that is *not* the actual Duecker model itself, but instead code changes that are used by the default model. These functions are not currently tested at all, and I have not added tests for them for similar reasons to jonescompneurolab#1185 (comment) After this, we should be able to make a PR that adds the actual Duecker model itself. However, there will be additional work for that step: we need to update `hnn_io.read_network_configuration` and the equivalent `...write...` such that they support the Duecker model. Continuing the pattern, this is based on top of jonescompneurolab#1187.

ntolley · 2025-11-12T22:50:33Z

hnn_core/cell.py

    Ra : float
        axial resistivity in ohm-cm
-    end_pts : list of [x, y, z]
+    v : float


Suggested change

v : float

v0 : float

Just a thought, with math notation an underscore zero typically denotes the initial value

ntolley · 2025-11-12T22:58:18Z

hnn_core/cells_default.py

            diam=dend_prop["diam"],
            Ra=dend_prop["Ra"],
            cm=dend_prop["cm"],
+            v=v,


any reason not to include v in the dend_prop dictionary?

hnn_core/cells_default.py

ntolley · 2025-11-13T12:27:02Z

@asoplata and @katduecker this looks quite clean to me!

Is it worth adding an explicit unit test that manually defines the initial membrane potential? The best test would be to record the membrane potential and make sure it matches the manually defined initial value

Perhaps the simplest approach would be to just add an ``assert vsec_data[0] == v` for one of the existing unit tests that records voltages?

github-project-automation bot added this to HNN Workspace Turbo 9000 Nov 6, 2025

github-project-automation bot moved this to Backlog in HNN Workspace Turbo 9000 Nov 6, 2025

asoplata marked this pull request as draft November 6, 2025 22:36

asoplata changed the title ~~[WIP] feat: Part 1 of many of extracting KD's v_init~~ [WIP] feat: Extracting Katharina's voltage-initialization changes Nov 7, 2025

asoplata marked this pull request as ready for review November 7, 2025 14:25

asoplata changed the title ~~[WIP] feat: Extracting Katharina's voltage-initialization changes~~ [MRG] feat: Extracting Katharina's voltage-initialization changes Nov 7, 2025

asoplata force-pushed the kd/initial_voltage_fix branch from a7833bd to a0acfdb Compare November 7, 2025 14:37

asoplata requested review from dylansdaniels, katduecker and ntolley November 7, 2025 14:39

katduecker and others added 4 commits November 7, 2025 09:45

kd: apply cells_default.py changes [no ci]

b8953da

This does NOT pass tests The original person who actually wrote this code was @katduecker, which is why they are indicated as author. This version of the code that was extracted from jonescompneurolab#1168 was done so by @asoplata .

asoplata force-pushed the kd/initial_voltage_fix branch from a0acfdb to 80863e1 Compare November 7, 2025 14:46

asoplata mentioned this pull request Nov 7, 2025

[WIP] feat: add KD's new Ca-baseline-correction #1187

Closed

asoplata mentioned this pull request Nov 7, 2025

[WIP]: feat: KD add remaining extracted non-model code #1188

Open

katduecker reviewed Nov 12, 2025

View reviewed changes

ntolley reviewed Nov 12, 2025

View reviewed changes

hnn_core/cells_default.py Show resolved Hide resolved



		def _get_pyr_soma(p_all, cell_type):
		# KD: In the new model, the basal dendrites are differently tuned from the apical

[MRG] feat: Extracting Katharina's voltage-initialization changes #1185

Are you sure you want to change the base?

[MRG] feat: Extracting Katharina's voltage-initialization changes #1185

Uh oh!

Conversation

asoplata commented Nov 6, 2025

Uh oh!

asoplata commented Nov 7, 2025

Uh oh!

asoplata commented Nov 7, 2025

Uh oh!

asoplata commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asoplata commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katduecker Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

ntolley Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

katduecker Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

katduecker Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

ntolley Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

katduecker commented Nov 12, 2025

Uh oh!

ntolley Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

ntolley Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ntolley commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asoplata commented Nov 7, 2025 •

edited

Loading

asoplata commented Nov 7, 2025 •

edited

Loading