[WIP] feat: Extracting Katharina's voltage-initialization changes #1185
Conversation
asoplata
changed the title
asoplata
changed the title
|
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 I was planning on writing tests for the new voltage-initialization, but as it turns out we have very, very little testing of However, since #1168 includes a large number of changes to |
|
This will resolve #996 . |
asoplata
force-pushed
the
kd/initial_voltage_fix
branch
from
a7833bd to
a0acfdb
Compare
|
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). |
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 .
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 .
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 .
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
force-pushed
the
kd/initial_voltage_fix
branch
from
a0acfdb to
80863e1
Compare
|
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.
|
@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.
|
@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? |
fix: bug in v->v0 rename This is a good reason WHY we should perform the name change in this case. yet another v->v0 fix
|
asoplata
changed the title
|
|
||
| def _get_dends(params, cell_type, section_names): | ||
| """Convert a flat dictionary to a nested dictionary. | ||
| def _get_dends( |
There was a problem hiding this comment.
Hey @katduecker , what do you think about this refactor?
In your existing Duecker 2025 model branch (at #1168), it seems like your new _get_basal function is the same as the old _get_dends, except that the variable middle gets a new value in the basal cell case. So I thought, instead of making a whole new function for mostly the same functionality, let's add a flag that distinguishes these two values, and is only expected to be used by your new model. Is this okay with you?
Also, since I've decided to use this opportunity to upgrade the documentation in cells_default.py, I'm also moving the 2? comments that you previously made as comments into the actual docstrings where they apply instead.
3 participants
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:
master(this PR),master,master) back into the code at [WIP] Experimental duecker_ET model branch #1168 .For starters, this first commit consists of only the extracted changes to
cell.pyand nothing else. Fortunately, in this case, the tests seem to pass without having to change anything.