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] GUI synaptic gains implementation #918

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

Conversation

gtdang
Copy link
Collaborator

@gtdang gtdang commented Oct 22, 2024

Added synaptic gains widgets to the GUI.

Changes:

  1. Adjusted Network class methods
    2. Renamed update_weights method to set_synaptic_gains for more clarity
    3. Added get_synaptic_gains method and test
  2. Added new "Synaptic gains" child tab under the Network tab with widgets for each weight
  3. Updated the _init_network_from_widgets function to update synaptic gains
  4. Added tests for GUI synaptic gains
  5. Corrected spelling error for cell parameters variable

Screenshot 2024-10-22 at 2 27 42 PM

@gtdang gtdang marked this pull request as ready for review October 22, 2024 19:03
@asoplata
Copy link
Collaborator

Did some initial simulation tests using the new tab, and the simulated changes do appear to have the expected effects. Still need to actually review both the code and the tests.

@asoplata
Copy link
Collaborator

Please see #915 for some discussion on planning this for the GUI.

Also, @dylansdaniels had a great idea: Instead of creating a new Synaptic gains Network tab, we could instead add the values to the top of Connectivity. This way, once we have each individual connection's gain also displayed in the Connectivity tab (currently a TODO), everything for the gains would be in the same location and with less clutter. Additionally, we could also add a button nearby with something like "Apply global synaptic gain changes" which applies a change via one of the four above gains, but then the user can inspect the current, actual value of the gain below on a per-connection basis.

I will try to add the per-connection gain to the Connectivity tab on this branch.

@gtdang
Copy link
Collaborator Author

gtdang commented Oct 22, 2024

Please see #915 for some discussion on planning this for the GUI.

Also, @dylansdaniels had a great idea: Instead of creating a new Synaptic gains Network tab, we could instead add the values to the top of Connectivity. This way, once we have each individual connection's gain also displayed in the Connectivity tab (currently a TODO), everything for the gains would be in the same location and with less clutter. Additionally, we could also add a button nearby with something like "Apply global synaptic gain changes" which applies a change via one of the four above gains, but then the user can inspect the current, actual value of the gain below on a per-connection basis.

I will try to add the per-connection gain to the Connectivity tab on this branch.

Would that involve a change to the API to set per-connection gain?

@asoplata
Copy link
Collaborator

I don't think it would need a change to the API, since it should be analogous to changing nc_dict's A_weight value. Related question: is this https://github.com/jonescompneurolab/hnn-core/blob/master/hnn_core/gui/gui.py#L1818-L1819 where the GUI actually applies weight changes from the GUI to the Net object?

@asoplata
Copy link
Collaborator

Noticed a small bug: Loading in a new network connectivity file adds another full set of the 4 connection gain types:
Screenshot_20241022_172217

@gtdang
Copy link
Collaborator Author

gtdang commented Oct 22, 2024

Yes, but you don't want to be applying the gain and updating the weight there. The nc_dict has a gain parameter. The multiplication should be left to the Network at simulation runtime.

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.

Looks good! Just left tiny nitpicks ... I didn't test though. I am hoping @dylansdaniels or @asoplata took care of that ;)

hnn_core/network.py Outdated Show resolved Hide resolved
def _get_cell_index_by_synapse_type(net):
"""Returns the indexes of excitatory and inhibitory cells in the network.

This function extracts the source GIDs (Global Identifiers) of excitatory
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Global Identifiers" isn't too descriptive either ... it's a bit of a misnomer since it's mainly a programming construct. I like to say "cell ID" to be clear.

return np.concatenate([list(net.connectivity[conn_idx]['src_gids'])
for conn_idx in indices]).tolist()

e_conns = pick_connection(net, receptor=['ampa', 'nmda'])
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
e_conns = pick_connection(net, receptor=['ampa', 'nmda'])
picks_e = pick_connection(net, receptor=['ampa', 'nmda'])

I like to use e_conn after the indexing, i.e.:

e_conns = [net.connectivity[p] for p in picks_e]

so it's clear from the variable name that one is an element of net.connectivity and the other is simply an index

@@ -1719,6 +1737,24 @@ def add_cell_parameters_tab(cell_params_out, cell_pameters_vboxes,
cell_layer_radio_button.value)


def add_synaptic_gain_tab(net, syn_gain_out, syn_gain_textfields, layout):
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing docstring!

hnn_core/network.py Outdated Show resolved Hide resolved
}

# Retrieve the gain value for each connection type
for conn_type, (src_indexes, target_indexes) in conn_types.items():
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
for conn_type, (src_indexes, target_indexes) in conn_types.items():
for conn_type, (src_idxs, target_idxs) in conn_types.items():

just for sake of consistency


# Retrieve the gain value for each connection type
for conn_type, (src_indexes, target_indexes) in conn_types.items():
conn_indices = pick_connection(self,
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
conn_indices = pick_connection(self,
picks = pick_connection(self,

see above comment too ... would try to be consistent with naming

src_gids=src_indexes,
target_gids=target_indexes)

if conn_indices:
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
if conn_indices:
if len(picks) > 0:

to be explicit that picks is a list, not a bool

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 using the truthyness of collections is more concise and pythonic. If we really wanted to be explicit we could also name the variable "picks_list".

But I'll open it up to the rest of the group to define a style guide. @ntolley @asoplata @dylansdaniels

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