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 method to modify synaptic gain #897

Merged
merged 14 commits into from
Sep 27, 2024

Conversation

ntolley
Copy link
Contributor

@ntolley ntolley commented Sep 23, 2024

Closes #885. This PR allows updating synaptic weights by a scalar gain parameter. Synaptic weights are grouped by E/I connections types (EE, EI, IE, II).

Updates can be performed as:

# In place modification
net.update_weights(e_e=2.0, copy=False)

# Return a copy with updated gains
net2 = net.update_weights(e_e=2.0, copy=True

Synaptic gain of inhibitory to inhibitory connections (default 1.0)
copy : bool
If True, create a copy of the network. If False, update the network
in place (default True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

returns ?

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.

Looks good! Had a few suggestions for additional tests.

hnn_core/tests/test_network.py Show resolved Hide resolved
hnn_core/network_builder.py Show resolved Hide resolved
@ntolley
Copy link
Contributor Author

ntolley commented Sep 23, 2024

@gtdang I'm realizing now that our io tests are rather brittle. If any future PR's modify attributes of the network, the default .json files will also need to be updated, perhaps we should look into streamlining this a bit more?

@gtdang
Copy link
Collaborator

gtdang commented Sep 23, 2024

@gtdang I'm realizing now that our io tests are rather brittle. If any future PR's modify attributes of the network, the default .json files will also need to be updated, perhaps we should look into streamlining this a bit more?

There's a function a the top of the tests called generate_test_files that will regenerate the json. You have to run it manually and commit the updated file with the PR though. We could made it a fixture that regenerates it every time the file is run, but the tradeoff is a longer runtime. Or we can put it at the very top and add a note that it must be re-run if the Network structure is changed.

Ultimately if the network structure is altered, we'll likely also need to update the IO functions that read/write the network to file, so the IO tests should break.

@ntolley ntolley changed the title WIP: Add method to modify synaptic gain [MRG] Add method to modify synaptic gain Sep 27, 2024
@gtdang gtdang self-requested a review September 27, 2024 16:05
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.

Everything looks good to me. Though I think we did discuss about changing the method so that it only reassigns the gains specified instead of resetting the unspecified ones to 1. Is that something we still want to do?

@gtdang gtdang self-requested a review September 27, 2024 19:15
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.

Looks good to me! Was there anything more you wanted to do with this PR? Otherwise I'm ok with merging.

@gtdang gtdang self-requested a review September 27, 2024 19:19
@@ -1428,6 +1431,75 @@ def add_electrode_array(self, name, electrode_pos, *, conductivity=0.3,
method=method,
min_distance=min_distance)})

def update_weights(self, e_e=None, e_i=None,
i_e=None, i_i=None, copy=True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the default behavior of the method be copy=False? Will most use cases be updating the weights in-place or saving to a new object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed! will change to false and then good to merge

@gtdang gtdang merged commit 7c4d560 into jonescompneurolab:master Sep 27, 2024
12 checks passed
@@ -1428,6 +1431,76 @@ def add_electrode_array(self, name, electrode_pos, *, conductivity=0.3,
method=method,
min_distance=min_distance)})

def update_weights(self, e_e=None, e_i=None,
i_e=None, i_i=None, copy=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

something I realized late ... might be worth adding this to an example to show off that it's possible to do. Otherwise it's kind of hidden from the user.

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.

API: Synaptic gains
3 participants