[MRG] Add name parameter to tonic bias and add tonic biases to different sections #922
Conversation
katduecker
changed the title
|
@katduecker can you add some unit tests? |
Yep, done! User now receives a warning when they don't define a section, and there will be an error if they define a section that's not part of the cell. I used a DeprecationWarning because Userwarnings don't seem to be raised on my computer so the test would fail. I mentioned this before that UserWarnings aren't raised by default on all systems. Somehow connection to NEURON timed out? Apart from that I'm ready for code review |
katduecker
changed the title
hnn_core/network.py
Outdated
| if section is None: | ||
| warnings.warn("Section for tonic bias is not defined." | ||
| "Defaulting to 'soma'.", | ||
| DeprecationWarning, stacklevel=1) | ||
| # error when section is defined that doesn't exist. | ||
| elif section not in sections: | ||
| raise ValueError(f'section must be an existing ' | ||
| f'section of the current cell or None. ' | ||
| f'Got {section}.') | ||
| else: | ||
| cell_type_bias['section'] = section |
There was a problem hiding this comment.
Where is it defaulting to soma?
There was a problem hiding this comment.
I realize now that it just doesn't add 'section' to the cell_type_bias dictionary and the Cell.create_tonic_bias will eventually use 'soma' as a default. I feel like that behavior is obtuse. Is it possible to bring the defaults higher-level?
There was a problem hiding this comment.
See above, it's because of test_read_configuration_json
There was a problem hiding this comment.
Resolved in developer meeting. We ended up regenerating the jones2009_3x3_drives.json file so that it contains section. The user will not be affected by this change if they don't define a section.
hnn_core/tests/test_gui.py
Outdated
| # OK to ignore section here? | ||
| ignore_keys=['tstop', 'section'], |
There was a problem hiding this comment.
We'll have to update the GUI for this tonic bias API change. I think what would make the most sense is creating a side branch for "multiple_tonics". We'll merge this PR into that branch and then add a second PR with the GUI changes. Once both are combined and working, we can merge it into main. Thoughts @asoplata @jasmainak @ntolley @dylansdaniels ?
There was a problem hiding this comment.
is the multiple tonic feature required in the GUI? In any case, I think it could be a follow-up PR ... wouldn't make side branches, it makes for more complicated workflows
There was a problem hiding this comment.
If someone tired to load a network configuration file with multiple tonics it wouldn't work. I'm not exactly sure what would happen but I think there are checks to make sure that only one tonic is allowed and it must be named a certain way.
There was a problem hiding this comment.
Just a few more tweaks @katduecker ...
Also don't forget to update whats_new.rst !
Co-authored-by: Mainak Jas <jasmainak@users.noreply.github.com>
Co-authored-by: Mainak Jas <jasmainak@users.noreply.github.com>
Co-authored-by: Mainak Jas <jasmainak@users.noreply.github.com>
Co-authored-by: Mainak Jas <jasmainak@users.noreply.github.com>
Co-authored-by: Mainak Jas <jasmainak@users.noreply.github.com>
|
Thanks Mainak, done! |
|
I enabled auto-merge once CIs pass. Thanks @katduecker ! 🥳 |
This issue was opened by @gtdang as #768. I have added the option to name a tonic bias and define the section where the tonic bias should be applied.
I need this feature for some of the tuning that I want to do based on literature on patch clamp recordings. In my case, I want the L5 pyramidal neuron to behave differently for somatic and apical stimulation (and stimulating both at the same time).
I hope this is useful!