-
Notifications
You must be signed in to change notification settings - Fork 55
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 cell parameters in GUI #806
Conversation
2701373
to
9510844
Compare
@@ -220,7 +234,7 @@ | |||
"metadata": {}, | |||
"outputs": [], | |||
"source": [ | |||
"gui._simulate_left_tab_click(\"Network connectivity\")" | |||
"gui._simulate_left_tab_click(\"Network\")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're going to move away from the automatically generated GUI documentation so not necessary to update this file (no need to remove this change of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one of the tests fails if the 'Network Connectivity' tab title changes but it's not updated in the notebook.
hnn_core/gui/gui.py
Outdated
dendrite_sections = [ | ||
'apical_trunk', 'apical_1', 'apical_tuft', 'apical_oblique', | ||
'basal_1', 'basal_2', 'basal_3' | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dendrite_sections = [ | |
'apical_trunk', 'apical_1', 'apical_tuft', 'apical_oblique', | |
'basal_1', 'basal_2', 'basal_3' | |
] | |
dendrite_sections = [name for name in sections.keys() if name != 'soma'] |
This list is actually missing apical_2
so perhaps it's better to just loop over the section names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
connectivity_names = ( | ||
'Layer 2/3 Pyramidal', 'Layer 5 Pyramidal', 'Layer 2 Basket', | ||
'Layer 5 Basket') | ||
cell_connectivity = Accordion(children=connectivity_boxes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this somehow redundant or being unused? I see that you're using add_connectivity_tab
to setup the connectivity, but that function already existed before this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think it was being used at all. That variable only exists during the scope of that function.
c653d6c
to
3f1abf5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all good on my end @kmilo9999, really amazing work here!!
@gtdang feel free to merge if everything looks good to you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have some minor comments but otherwise approve.
hnn_core/tests/test_gui.py
Outdated
for cell_layer in layers: | ||
key = f'{cell_type} Pyramidal_{cell_layer}' | ||
assert (any(key in k for k in keys)) | ||
num_cell_params = num_cell_params + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
num_cell_params = num_cell_params + 1 | |
num_cell_params += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
assert (len(keys) == num_cell_params) | ||
|
||
# Check the if the cell params dictionary has been updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Check the if the cell params dictionary has been updated | |
# Security check for if parameters have been added or removed from the cell | |
# params dict. Any additions will need mappings added to the | |
# update_{*}_cell_params functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
hnn_core/gui/gui.py
Outdated
for vbox_key, cell_param_list in cell_params_vboxes.items(): | ||
for key, update_function in update_functions.items(): | ||
if key in vbox_key: | ||
# Remove 'Pyramidal' keyword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it.
|
||
import base64 | ||
import zipfile | ||
import numpy as np | ||
from copy import deepcopy | ||
|
||
cell_parameters_dict = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A suggestion for a future PR... Changing these to dictionaries with index at the keys and the tuples as the values would help with configuring and debugging the parameter mappings in the update_*_cell_params
functions assuming there is a 1:1 order mapping of widgets to these lists.
Amazing job @kmilo9999! |
This is the work for the cell params feature for L2 and L5 pyramidal cell types