-
Notifications
You must be signed in to change notification settings - Fork 53
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
Conversation
hnn_core/network.py
Outdated
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) |
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.
returns ?
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.
Looks good! Had a few suggestions for additional tests.
@gtdang I'm realizing now that our |
There's a function a the top of the tests called 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. |
… in the __eq__ method of the Network class
This function is a stand-in function for the __eq__ method used only in the tests.
…d correctly by the set gains
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.
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?
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.
Looks good to me! Was there anything more you wanted to do with this PR? Otherwise I'm ok with merging.
hnn_core/network.py
Outdated
@@ -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): |
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.
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?
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.
agreed! will change to false and then good to merge
@@ -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): |
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.
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.
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: