Merge pull request #3021 from heplesser/fix_conn_common

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

models/jonke_synapse.h

@@ -209,13 +209,6 @@ class jonke_synapse : public Connection< targetidentifierT >
    */
   void set_status( const DictionaryDatum& d, ConnectorModel& cm );
 
-  /**
-   * Checks to see if illegal parameters are given in syn_spec.
-   *
-   * The illegal parameters are:  "alpha", "beta", "lambda", "mu_plus", "mu_minus", "tau_plus", "Wmax"
-   */
-  void check_synapse_params( const DictionaryDatum& d ) const;
-
   /**
    * Send an event to the receiver of this connection.
    * \param e The event to send
@@ -390,24 +383,6 @@ jonke_synapse< targetidentifierT >::set_status( const DictionaryDatum& d, Connec
   }
 }
 
-template < typename targetidentifierT >
-void
-jonke_synapse< targetidentifierT >::check_synapse_params( const DictionaryDatum& syn_spec ) const
-{
-  std::string param_arr[] = { "alpha", "beta", "lambda", "mu_plus", "mu_minus", "tau_plus", "Wmax" };
-
-  const size_t n_param = sizeof( param_arr ) / sizeof( std::string );
-  for ( size_t n = 0; n < n_param; ++n )
-  {
-    if ( syn_spec->known( param_arr[ n ] ) )
-    {
-      std::string msg = "Connect doesn't support the setting of parameter " + param_arr[ n ]
-        + " in jonke_synapse. Use SetDefaults() or CopyModel().";
-      throw NotImplemented( msg );
-    }
-  }
-}
-
 } // of namespace nest
 
 #endif // of #ifndef JONKE_SYNAPSE_H

models/stdp_dopamine_synapse.h

@@ -393,13 +393,6 @@ template < typename targetidentifierT >
 void
 stdp_dopamine_synapse< targetidentifierT >::check_synapse_params( const DictionaryDatum& syn_spec ) const
 {
-  if ( syn_spec->known( names::volume_transmitter ) )
-  {
-    throw NotImplemented(
-      "Connect doesn't support the direct specification of the "
-      "volume transmitter of stdp_dopamine_synapse in syn_spec."
-      "Use SetDefaults() or CopyModel()." );
-  }
   // Setting of parameter c and n not thread safe.
   if ( kernel().vp_manager.get_num_threads() > 1 )
   {
@@ -418,18 +411,6 @@ stdp_dopamine_synapse< targetidentifierT >::check_synapse_params( const Dictiona
         "Use SetDefaults() or CopyModel()." );
     }
   }
-  std::string param_arr[] = { "A_minus", "A_plus", "Wmax", "Wmin", "b", "tau_c", "tau_n", "tau_plus" };
-
-  const size_t n_param = sizeof( param_arr ) / sizeof( std::string );
-  for ( size_t n = 0; n < n_param; ++n )
-  {
-    if ( syn_spec->known( param_arr[ n ] ) )
-    {
-      std::string msg = "Connect doesn't support the setting of parameter " + param_arr[ n ]
-        + " in stdp_dopamine_synapse. Use SetDefaults() or CopyModel().";
-      throw NotImplemented( msg );
-    }
-  }
 }
 
 template < typename targetidentifierT >

models/tsodyks_synapse_hom.h

@@ -251,10 +251,9 @@ class tsodyks_synapse_hom : public Connection< targetidentifierT >
   void
   set_weight( double )
   {
-    throw BadProperty(
+    throw NotImplemented(
       "Setting of individual weights is not possible! The common weights can "
-      "be changed via "
-      "CopyModel()." );
+      "be changed via CopyModel()." );
   }
 
 private:

nestkernel/connection.h

@@ -158,6 +158,8 @@ class Connection
    *
    * @note Classes requiring checks need to override the function with their own
    * implementation, as this base class implementation does not do anything.
+   *
+   * @see ConnectorModel::check_synapse_params
    */
   void check_synapse_params( const DictionaryDatum& d ) const;
 

nestkernel/connector_model.h

@@ -111,6 +111,10 @@ class ConnectorModel
 
   /**
    * Checks to see if illegal parameters are given in syn_spec.
+   *
+   * Checks against setting CommonSynapseProperties upon Connect() are implemented in GenericConnectorModel.
+   * Any further checks need to be implemented by the connection model class by overriding
+   * Connection::check_synapse_params().
    */
   virtual void check_synapse_params( const DictionaryDatum& ) const = 0;
 
@@ -182,12 +186,6 @@ class GenericConnectorModel : public ConnectorModel
   void get_status( DictionaryDatum& ) const override;
   void set_status( const DictionaryDatum& ) override;
 
-  void
-  check_synapse_params( const DictionaryDatum& syn_spec ) const override
-  {
-    default_connection_.check_synapse_params( syn_spec );
-  }
-
   typename ConnectionT::CommonPropertiesType const&
   get_common_properties() const override
   {
@@ -196,6 +194,8 @@ class GenericConnectorModel : public ConnectorModel
 
   void set_syn_id( synindex syn_id ) override;
 
+  void check_synapse_params( const DictionaryDatum& syn_spec ) const override;
+
   SecondaryEvent*
   get_secondary_event() override
   {

nestkernel/connector_model_impl.h

@@ -137,6 +137,28 @@ GenericConnectorModel< ConnectionT >::set_status( const DictionaryDatum& d )
   default_delay_needs_check_ = true;
 }
 
+template < typename ConnectionT >
+void
+GenericConnectorModel< ConnectionT >::check_synapse_params( const DictionaryDatum& syn_spec ) const
+{
+  // This is called just once per Connect() call, so we need not worry much about performance.
+  // We get a dictionary with synapse default values and check if any of its keys are in syn_spec.
+  DictionaryDatum dummy( new Dictionary );
+  cp_.get_status( dummy );
+
+  for ( [[maybe_unused]] const auto& [ key, val ] : *syn_spec )
+  {
+    if ( dummy->known( key ) )
+    {
+      throw NotImplemented(
+        String::compose( "Synapse parameter \"%1\" can only be set via SetDefaults() or CopyModel().", key ) );
+    }
+  }
+
+  default_connection_.check_synapse_params( syn_spec );
+}
+
+
 template < typename ConnectionT >
 void
 GenericConnectorModel< ConnectionT >::used_default_delay()

testsuite/pytests/sli2py_connect/test_common_properties_setting.py

@@ -0,0 +1,135 @@
+# -*- coding: utf-8 -*-
+#
+# test_common_properties_setting.py
+#
+# This file is part of NEST.
+#
+# Copyright (C) 2004 The NEST Initiative
+#
+# NEST is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 2 of the License, or
+# (at your option) any later version.
+#
+# NEST is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with NEST.  If not, see <http://www.gnu.org/licenses/>.
+
+"""
+Test that common properties can be Set/Read via Defaults, but not individual connections.
+
+These tests only check the setting/getting mechanics for one parameter per synapse to confirm
+that the overall mechanics "work". Testing the setting of specific parameters must be done in
+model-specific tests.
+"""
+
+import nest
+import pytest
+
+
+def set_volume_transmitter():
+    vt = nest.Create("volume_transmitter")
+    nest.SetDefaults("stdp_dopamine_synapse", {"volume_transmitter": vt})
+
+
+def set_default_delay_resolution():
+    nest.resolution = nest.GetDefaults("eprop_synapse")["delay"]
+
+
+# This list shall contain all synapse models extending the CommonSynapseProperties class.
+# For each model, specify which parameter to test with and which test value to use. A
+# setup function can be provided if preparations are required. Provide also supported neuron model.
+common_prop_models = {
+    "eprop_synapse": {
+        "parameter": "batch_size",
+        "value": 10,
+        "setup": set_default_delay_resolution,
+        "neuron": "eprop_iaf_psc_delta",
+    },
+    "jonke_synapse": {"parameter": "tau_plus", "value": 10, "setup": None, "neuron": "iaf_psc_alpha"},
+    "stdp_dopamine_synapse": {
+        "parameter": "tau_plus",
+        "value": 10,
+        "setup": set_volume_transmitter,
+        "neuron": "iaf_psc_alpha",
+    },
+    "stdp_facetshw_synapse_hom": {"parameter": "tau_plus", "value": 10, "setup": None, "neuron": "iaf_psc_alpha"},
+    "stdp_pl_synapse_hom": {"parameter": "tau_plus", "value": 10, "setup": None, "neuron": "iaf_psc_alpha"},
+    "stdp_synapse_hom": {"parameter": "tau_plus", "value": 10, "setup": None, "neuron": "iaf_psc_alpha"},
+    "static_synapse_hom_w": {"parameter": "weight", "value": 10, "setup": None, "neuron": "iaf_psc_alpha"},
+    "tsodyks_synapse_hom": {"parameter": "tau_psc", "value": 10, "setup": None, "neuron": "iaf_psc_alpha"},
+}
+
+# Filter models that may not be built in
+available_cp_models = {k: v for k, v in common_prop_models.items() if k in nest.synapse_models}
+
+
+@pytest.fixture(autouse=True)
+def reset_kernel():
+    nest.ResetKernel()
+
+
+@pytest.mark.parametrize("syn_model, specs", available_cp_models.items())
+def test_set_common_properties(syn_model, specs):
+    """Test that setting a parameter works"""
+
+    if specs["setup"]:
+        specs["setup"]()
+
+    old_val = nest.GetDefaults(syn_model)[specs["parameter"]]
+    assert old_val != specs["value"]  # test would be meaningless otherwise
+
+    nest.SetDefaults(syn_model, {specs["parameter"]: specs["value"]})
+    new_val = nest.GetDefaults(syn_model)[specs["parameter"]]
+    assert new_val == specs["value"]
+
+
+@pytest.mark.parametrize("syn_model, specs", available_cp_models.items())
+def test_copy_common_properties(syn_model, specs):
+    """Test that parameters set on a model are copied and that setting on the copy does not touch the original"""
+
+    if specs["setup"]:
+        specs["setup"]()
+
+    old_val = nest.GetDefaults(syn_model)[specs["parameter"]]
+    assert old_val != specs["value"]  # test would be meaningless otherwise
+
+    nest.SetDefaults(syn_model, {specs["parameter"]: specs["value"]})
+    new_model = syn_model + "_copy"
+    nest.CopyModel(syn_model, new_model)
+    new_val = nest.GetDefaults(new_model)[specs["parameter"]]
+    assert new_val == specs["value"]
+
+    # Set parameter back on copied model, original must not be changed
+    nest.SetDefaults(new_model, {specs["parameter"]: old_val})
+    assert nest.GetDefaults(syn_model)[specs["parameter"]] == specs["value"]
+
+
+@pytest.mark.parametrize("syn_model, specs", available_cp_models.items())
+def test_no_setting_on_connection(syn_model, specs):
+    """Test that common property cannot be set on individual connection"""
+
+    if specs["setup"]:
+        specs["setup"]()
+
+    n = nest.Create(specs["neuron"])
+    nest.Connect(n, n, syn_spec={"synapse_model": syn_model})
+    conn = nest.GetConnections()
+    with pytest.raises(nest.kernel.NESTErrors.DictError):
+        conn.set({specs["parameter"]: specs["value"]})
+
+
+@pytest.mark.parametrize("syn_model, specs", available_cp_models.items())
+def test_no_setting_on_connect(syn_model, specs):
+    """Test that common property cannot be set in a Connect call"""
+
+    if specs["setup"]:
+        specs["setup"]()
+
+    n = nest.Create(specs["neuron"])
+    with pytest.raises(nest.kernel.NESTErrors.NotImplemented):
+        nest.Connect(n, n, syn_spec={"synapse_model": syn_model, specs["parameter"]: specs["value"]})