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

Ensure Connect() does not allow passing parameters that need to be set at the synapse-model level #3021

Merged
merged 8 commits into from
Nov 30, 2023

Conversation

heplesser
Copy link
Contributor

Until now, users could pass parameters shared by all synapses of a given type to the syn_spec in Connect(). These parameters were silently ignored as illustrated by the example below, where alpha is not set to 10 either in the connection (which does not have that property) nor in the defaults.

The PR also ports the pertaining tests to Python, in part simplifying and in part extending them.

In [2]: n = nest.Create('iaf_psc_alpha')
In [4]: nest.Connect(n, n, syn_spec={'synapse_model': 'stdp_synapse_hom', 'alpha': 10})
In [5]: c = nest.GetConnections()
In [6]: c.get()
Out[6]: 
{'delay': 1.0,
 'Kplus': 0.0,
 'port': 0,
 'receptor': 0,
 'sizeof': 48,
 'source': 1,
 'synapse_id': 54,
 'synapse_model': 'stdp_synapse_hom',
 'target': 1,
 'target_thread': 0,
 'weight': 1.0}

In [7]: nest.GetDefaults('stdp_synapse_hom')
Out[7]: 
{'alpha': 1.0,
 'delay': 1.0,
 'element_type': 'synapse',
 'has_delay': True,
 'Kplus': 0.0,
 'lambda': 0.01,
 'mu_minus': 1.0,
 'mu_plus': 1.0,
 'num_connections': 1,
 'receptor_type': 0,
 'requires_symmetric': False,
 'sizeof': 48,
 'synapse_model': 'stdp_synapse_hom',
 'synapse_modelid': 54,
 'tau_plus': 20.0,
 'Wmax': 100.0,
 'weight': 1.0,
 'weight_recorder': NodeCollection(<empty>)}

@heplesser heplesser added T: Bug Wrong statements in the code or documentation S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Nov 28, 2023
@heplesser heplesser added this to In progress in Kernel via automation Nov 28, 2023
Copy link
Contributor

@akorgor akorgor left a comment

Choose a reason for hiding this comment

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

This PR prevents the scenario that users pass common synapse properties directly to the syn_spec in Connect() and don't notice that these are ignored by throwing an error suggesting to set the respective parameter via SetDefaults on the synpase model, which is an important fix.

The mechanism and the test work like a charm and the code is clear and well-documented.

…g.py

Co-authored-by: Agnes Korcsak-Gorzo <40828647+akorgor@users.noreply.github.com>
Copy link
Member

@nicolossus nicolossus left a comment

Choose a reason for hiding this comment

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

@heplesser Thanks! This looks mostly good to me, I only have a few remarks/questions. See inline comments.

models/tsodyks_synapse_hom.h Show resolved Hide resolved
models/tsodyks_synapse_hom.h Outdated Show resolved Hide resolved
nestkernel/connector_model_impl.h Show resolved Hide resolved
Kernel automation moved this from In progress to PRs pending approval Nov 29, 2023
heplesser and others added 3 commits November 29, 2023 21:55
Copy link
Member

@nicolossus nicolossus left a comment

Choose a reason for hiding this comment

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

LGTM!

@heplesser heplesser merged commit 453373f into nest:master Nov 30, 2023
24 checks passed
Kernel automation moved this from PRs pending approval to Done Nov 30, 2023
@heplesser heplesser deleted the fix_conn_common branch April 24, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Bug Wrong statements in the code or documentation
Projects
Kernel
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants