[MRG] REF/FIX consolidate v_inits and use on read - #1327
Conversation
This makes two changes for the sake of fixing a current bug/regression on `master` that was introduced via https://github.com/jonescompneurolab/hnn-core/blob/5ca983a3dd53b6e749c377b24bcba64773debbd3/hnn_core/hnn_io.py#L174 as part of jonescompneurolab#1185. 1. In `hnn_io.py`, when we introduced Katharina's voltage-initialization updates, I included some backwards-compatibility code that was incorrect. - The above code in `_read_cell_types`, when reading in an older Network JSON file that does not have a `v0` entry (which is any Network JSON file created before jonescompneurolab#1185 was merged), would default to setting that cell type's `v0` to be -65. This was probably a misunderstanding on my part at the time of how `h.finitialize` works. Currently, on `master`, this leads to an undetected bug that @annacatt found, where older Network JSON files such as https://github.com/jonescompneurolab/hnn-data/blob/main/workshops/2025-04-09-HNN-online_workshop/gamma_gui_walkthrough/gamma_L5weak_L2weak.json have their voltages initialized to -65, which is NOT correct. Previously, since there was no `v0` key in the Network JSON, these voltages would be initialized at simulation time to their proper, diverse values. - In order to fix this while still supporting `v0` usage in Network JSON files, when a Network JSON is read in, it needs to have its correct, corresponding voltage initializtion applied to its `v0` key at read-in time. This is because `hnn_io._read_cell_types` itself creates the network's `Section` objects, instead of relying on `cells_default.py` to do so, which is now (after jonescompneurolab#1185) where voltage initialization takes place. - However, there's a small issue: currently, the different sections' hardcoded initial voltage values are embedded throughout `cells_default.py` itself. This leads to the next change in this PR: 2. So, in `cells_default.py`, similarly to how we manage default network metadata in `network_models.py`, I've extracted all the hardcoded initial voltage values to live in a module-level dictionary. Throughout `cells_default.py`, instead of the initial voltages living inside each subfunction, the subfunctions access the global dictionary. 3. Finally, this allows us to solve the original issue, which is, when reading in a Network JSON with `hnn_io`, we apply the hardcoded default initial voltage values for each default cell type. This approach is not perfect since it does visually group some parameters of the sections away from their individualized functions, but I think it's the best approach to be able to concisely communicate those initial voltage parameters cleanly to `hnn_io`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1327 +/- ##
==========================================
- Coverage 92.08% 92.04% -0.04%
==========================================
Files 28 28
Lines 7362 7341 -21
==========================================
- Hits 6779 6757 -22
- Misses 583 584 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| all_v_init = -71.46 | ||
| v_init = _default_v_init["L2_pyramidal"] | ||
|
|
||
| section_names = [ |
There was a problem hiding this comment.
Should there be a check to ensure that the keys of _default_v_init match section_names?
| Ra=section_data["Ra"], | ||
| v0=section_data.get("v0", -65), # for backwards compatibility | ||
| v0=section_data.get( | ||
| "v0", _default_v_init[cell_name][section_name] |
There was a problem hiding this comment.
Should the unit test be passing a cell_types_data dict where we explicitly .pop() the v0 element and ensure it updates to the default value in the dictionary?
There was a problem hiding this comment.
This is not found in a our current tests because the current .json correctly specify v0
I could definitely be convinced this doesn't need a unit test since these are the formats we are distributing going forward, I'll leave it to your call!
There was a problem hiding this comment.
I'm averse to popping the v0 in case this default dict gets used elsewhere later (which it does not currently)
There was a problem hiding this comment.
I mean specifically in a unit test where we load in a .json file
I made the comment in the spirit of writing a test that triggers the original bug that gave rise to the issue
Let me know if you think this and the other suggested test should be added, I can write them myself and push to the PR, if you don't think they're necessary then we can go ahead and merge
|
Great catch @asoplata! Just two small comments about unit tests and verifying metadata matches, everything else is good with me |
This is required since we don't want the notebook to be re-executed, due to this fix not being released jonescompneurolab/hnn-core#1327
* Add gamma tutorial python notebook * Add script for GUI tutorial recording and a changes+comments in python notebook * first austin checkin * fix note about connectivity, more nb work * save progress * Add maybe? final version of gamma nb * Add new gamma nb to skipped This is required since we don't want the notebook to be re-executed, due to this fix not being released jonescompneurolab/hnn-core#1327 * All other changes, including script deletion --------- Co-authored-by: Anna Cattani <annacattani@macbookpro.mynetworksettings.com> Co-authored-by: Austin E. Soplata <me@asoplata.com>
This makes two changes for the sake of fixing a current bug/regression on
masterthat was introduced viahnn-core/hnn_core/hnn_io.py
Line 174 in 5ca983a
In
hnn_io.py, when we introduced Katharina's voltage-initialization updates, I included some backwards-compatibility code that was incorrect.The above code in
_read_cell_types, when reading in an older Network JSON file that does not have av0entry (which is any Network JSON file created before [MRG] feat: Extracting Katharina's voltage-initialization changes #1185 was merged), would default to setting that cell type'sv0to be -65. This was probably a misunderstanding on my part at the time of howh.finitializeworks. Currently, onmaster, this leads to an undetected bug that @annacatt found, where older Network JSON files such as https://github.com/jonescompneurolab/hnn-data/blob/main/workshops/2025-04-09-HNN-online_workshop/gamma_gui_walkthrough/gamma_L5weak_L2weak.json have their voltages initialized to -65, which is NOT correct. Previously, since there was nov0key in the Network JSON, these voltages would be initialized at simulation time to their proper, diverse values.In order to fix this while still supporting
v0usage in Network JSON files, when a Network JSON is read in, it needs to have its correct, corresponding voltage initializtion applied to itsv0key at read-in time. This is becausehnn_io._read_cell_typesitself creates the network'sSectionobjects, instead of relying oncells_default.pyto do so, which is now (after [MRG] feat: Extracting Katharina's voltage-initialization changes #1185) where voltage initialization takes place.However, there's a small issue: currently, the different sections' hardcoded initial voltage values are embedded throughout
cells_default.pyitself. This leads to the next change in this PR:So, in
cells_default.py, similarly to how we manage default network metadata innetwork_models.py, I've extracted all the hardcoded initial voltage values to live in a module-level dictionary. Throughoutcells_default.py, instead of the initial voltages living inside each subfunction, the subfunctions access the global dictionary.Finally, this allows us to solve the original issue, which is, when reading in a Network JSON with
hnn_io, we apply the hardcoded default initial voltage values for each default cell type.This approach is not perfect since it does visually group some parameters of the sections away from their individualized functions, but I think it's the best approach to be able to concisely communicate those initial voltage parameters cleanly to
hnn_io.