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 cell parameters in GUI #806

Merged
merged 26 commits into from
Jul 13, 2024

Conversation

kmilo9999
Copy link
Collaborator

@kmilo9999 kmilo9999 commented Jul 3, 2024

This is the work for the cell params feature for L2 and L5 pyramidal cell types
cell_params

@kmilo9999 kmilo9999 marked this pull request as draft July 3, 2024 23:20
@kmilo9999 kmilo9999 linked an issue Jul 8, 2024 that may be closed by this pull request
@kmilo9999 kmilo9999 self-assigned this Jul 8, 2024
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
@kmilo9999 kmilo9999 changed the title [WIP]: Add cell parameters in GUI [ENH]: Add cell parameters in GUI Jul 10, 2024
@kmilo9999 kmilo9999 marked this pull request as ready for review July 10, 2024 17:02
@kmilo9999 kmilo9999 requested review from gtdang and ntolley July 10, 2024 17:02
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
@kmilo9999 kmilo9999 requested a review from ntolley July 10, 2024 20:05
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
hnn_core/gui/gui.py Outdated Show resolved Hide resolved
@@ -220,7 +234,7 @@
"metadata": {},
"outputs": [],
"source": [
"gui._simulate_left_tab_click(\"Network connectivity\")"
"gui._simulate_left_tab_click(\"Network\")"
Copy link
Contributor

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)

Copy link
Collaborator Author

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.

@ntolley
Copy link
Contributor

ntolley commented Jul 11, 2024

Looks like the test is failing here:
image

Comment on lines 1771 to 1774
dendrite_sections = [
'apical_trunk', 'apical_1', 'apical_tuft', 'apical_oblique',
'basal_1', 'basal_2', 'basal_3'
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@kmilo9999 kmilo9999 requested a review from ntolley July 11, 2024 19:22
connectivity_names = (
'Layer 2/3 Pyramidal', 'Layer 5 Pyramidal', 'Layer 2 Basket',
'Layer 5 Basket')
cell_connectivity = Accordion(children=connectivity_boxes)
Copy link
Contributor

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

Copy link
Collaborator Author

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.

@kmilo9999 kmilo9999 requested a review from ntolley July 12, 2024 13:35
Copy link
Contributor

@ntolley ntolley left a 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

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.

Have some minor comments but otherwise approve.

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
num_cell_params = num_cell_params + 1
num_cell_params += 1

Copy link
Collaborator Author

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a note?

Copy link
Collaborator Author

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 = {
Copy link
Collaborator

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.

@kmilo9999 kmilo9999 changed the title [ENH]: Add cell parameters in GUI [MRG]: Add cell parameters in GUI Jul 13, 2024
@gtdang gtdang merged commit b2a211a into jonescompneurolab:master Jul 13, 2024
12 checks passed
@gtdang
Copy link
Collaborator

gtdang commented Jul 13, 2024

Amazing job @kmilo9999!

@gtdang gtdang deleted the feat-cell-params branch July 13, 2024 14:44
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.

GUI cell parameters
4 participants