Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Add name parameter to tonic bias and add tonic biases to different sections #922

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

katduecker
Copy link
Collaborator

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!

@katduecker katduecker changed the title [MRG] Add name parameter to tonic bias and add tonic biases to different sections Add name parameter to tonic bias and add tonic biases to different sections Oct 28, 2024
@jasmainak
Copy link
Collaborator

@katduecker can you add some unit tests?

@katduecker
Copy link
Collaborator Author

@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 katduecker changed the title Add name parameter to tonic bias and add tonic biases to different sections [MRG] Add name parameter to tonic bias and add tonic biases to different sections Oct 30, 2024
Copy link
Collaborator

@gtdang gtdang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some questions and suggestions.

hnn_core/network.py Show resolved Hide resolved
hnn_core/network.py Outdated Show resolved Hide resolved
Comment on lines 1692 to 1702
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is it defaulting to soma?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, it's because of test_read_configuration_json

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 93 to 94
# OK to ignore section here?
ignore_keys=['tstop', 'section'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gtdang gtdang requested a review from asoplata October 30, 2024 14:59
hnn_core/network.py Outdated Show resolved Hide resolved
hnn_core/network.py Outdated Show resolved Hide resolved
@gtdang gtdang mentioned this pull request Nov 4, 2024
3 tasks
Copy link
Collaborator

@jasmainak jasmainak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more tweaks @katduecker ...

Also don't forget to update whats_new.rst !

hnn_core/tests/test_network.py Outdated Show resolved Hide resolved
hnn_core/tests/test_network.py Outdated Show resolved Hide resolved
hnn_core/tests/test_network.py Outdated Show resolved Hide resolved
hnn_core/network.py Outdated Show resolved Hide resolved
hnn_core/network.py Outdated Show resolved Hide resolved
@katduecker
Copy link
Collaborator Author

Thanks Mainak, done!

@jasmainak jasmainak enabled auto-merge (squash) November 7, 2024 19:15
@jasmainak
Copy link
Collaborator

I enabled auto-merge once CIs pass. Thanks @katduecker ! 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants