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

Fix synapse clamping #492

Merged
merged 9 commits into from
Nov 8, 2024
Merged

Fix synapse clamping #492

merged 9 commits into from
Nov 8, 2024

Conversation

jnsbck
Copy link
Contributor

@jnsbck jnsbck commented Nov 8, 2024

Clamping of synaptic states was not tested and slipped through the cracks when #447 was merged. This lead to several issues #485.

This PR fixes this and adds test coverage.

@@ -118,11 +118,11 @@ def add_stimuli(
if data_stimuli is not None:
externals["i"] = jnp.concatenate([externals["i"], data_stimuli[1]])
external_inds["i"] = jnp.concatenate(
[external_inds["i"], data_stimuli[2].global_comp_index.to_numpy()]
[external_inds["i"], data_stimuli[2].index.to_numpy()]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to .index, since edges does not have global_comp_index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...and clamping of synapses needs the dataframe index.

This was linked to issues Nov 8, 2024
@jnsbck jnsbck removed a link to an issue Nov 8, 2024
@jnsbck
Copy link
Contributor Author

jnsbck commented Nov 8, 2024

fyi @michaeldeistler
I will merge this since all tests are passing and @kyralianaka and @deezer257 rely on it. Please have a look nonetheless and poke me if I should touch some things up afterwards still.

Also @huangziwei I noticed we have #363 still open? I think this should be easy to do #487 is merged.

Comment on lines +1665 to +1669
all_externals = list(self.externals.keys())
if "i" in all_externals:
all_externals.remove("i")
state_names = all_externals if state_name is None else [state_name]
for state_name in state_names:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took the liberty of making state_name optional. If none is supplied delete_clamps() will just remove all clamps.

@jnsbck jnsbck mentioned this pull request Nov 8, 2024
@jnsbck jnsbck merged commit 0dad63e into main Nov 8, 2024
1 check passed
@jnsbck jnsbck deleted the fix_synapse_clamping branch November 8, 2024 13:26
michaeldeistler pushed a commit that referenced this pull request Nov 13, 2024
* add: wip version of tutorial on views

* fix: added fixes for clamping and stimulating of synapses

* fix: allow all states

* fix: add current to states

* fix: add tests for current and synapse clamping

* fix: fix current clamp test

* rm: rm accidently commited stash from different branch

* enh: allow to remove multiple clamp

* fix: make tests pass
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.

Recording of currents not possible Clamping of Synapse: Following Synapse might also be clamped
1 participant