SonataConfig.parsedModifications rework (#461)

## Context

modifications is now a list of dicts

Fix: #458 
Fix: #460 
Fix: #462

## Scope

- make `modifications` a list
- remove validator: it was validating just stuff coming from libsonata. Libsonata has the fields by construction
- adapt configuration and tests
- register modification types using an enum, not the name of the class

## Testing
- add test for #460

Author: cattabiani

neurodamus/core/configuration.py

@@ -254,7 +254,7 @@ def init(cls, config_file, cli_options):
         cls.connections = cls._config_parser.parsedConnects
         cls.stimuli = cls._config_parser.parsedStimuli
         cls.reports = cls._config_parser.parsedReports
-        cls.modifications = cls._config_parser.parsedModifications or {}
+        cls.modifications = cls._config_parser.parsedModifications
         cls.beta_features = cls._config_parser.beta_features
         cls.cli_options = CliOptions(**(cli_options or {}))
 
@@ -555,16 +555,6 @@ def _stimulus_params(config: _SimConfig):
         )
 
 
-@SimConfig.validator
-def _modification_params(config: _SimConfig):
-    required_fields = (
-        "Target",
-        "Type",
-    )
-    for name, mod_block in config.modifications.items():
-        _check_params("Modification " + name, mod_block, required_fields, ())
-
-
 def make_circuit_config(config_dict, req_morphology=True):
     if config_dict.get("CellLibraryFile") == "<NONE>":
         config_dict["CellLibraryFile"] = False

neurodamus/io/sonata_config.py

@@ -310,14 +310,7 @@ def parsedReports(self):
 
     @property
     def parsedModifications(self):
-        item_translation = {"node_set": "Target"}
-        result = {}
-        for modification in self._sim_conf.conditions.modifications():
-            setting = self._translate_dict(item_translation, modification)
-            self._adapt_libsonata_fields(setting)
-            result[modification.name] = setting
-
-        return result
+        return self._sim_conf.conditions.modifications()
 
     @staticmethod
     def _adapt_libsonata_fields(rep):

neurodamus/modification_manager.py

@@ -41,19 +41,18 @@ def __init__(self, target_manager):
         self._modifications = []
 
     def interpret(self, target_spec, mod_info):
-        mod_t_name = mod_info["Type"]
-        mod_t = self._mod_types.get(mod_t_name)
+        mod_t = self._mod_types.get(mod_info.type)
+
         if not mod_t:
-            raise ConfigurationError(f"Unknown Modification {mod_t_name}")
+            raise ConfigurationError(f"Unknown Modification {mod_info.type}")
         target = self._target_manager.get_target(target_spec)
         cell_manager = self._target_manager._cell_manager
         mod = mod_t(target, mod_info, cell_manager)
         self._modifications.append(mod)
 
     @classmethod
     def register_type(cls, mod_class):
-        """Registers a new class as a handler for a new modification type"""
-        cls._mod_types[mod_class.__name__] = mod_class
+        cls._mod_types[mod_class.MOD_TYPE] = mod_class
         return mod_class
 
 
@@ -64,7 +63,9 @@ class TTX:
     Uses TTXDynamicsSwitch as in BGLibPy. Overrides HOC version, which is outdated
     """
 
-    def __init__(self, target, mod_info: dict, cell_manager):
+    MOD_TYPE = libsonata.SimulationConfig.ModificationBase.ModificationType.TTX
+
+    def __init__(self, target, mod_info: libsonata.SimulationConfig.ModificationTTX, cell_manager):
         tpoints = target.get_point_list(
             cell_manager,
             section_type=libsonata.SimulationConfig.Report.Sections.all,
@@ -90,8 +91,15 @@ class ConfigureAllSections:
     Use case is modifying mechanism variables from config.
     """
 
-    def __init__(self, target, mod_info: dict, cell_manager):
-        config, config_attrs = self.parse_section_config(mod_info["SectionConfigure"])
+    MOD_TYPE = libsonata.SimulationConfig.ModificationBase.ModificationType.ConfigureAllSections
+
+    def __init__(
+        self,
+        target,
+        mod_info: libsonata.SimulationConfig.ModificationConfigureAllSections,
+        cell_manager,
+    ):
+        config, config_attrs = self.parse_section_config(mod_info.section_configure)
         tpoints = target.get_point_list(
             cell_manager,
             section_type=libsonata.SimulationConfig.Report.Sections.all,

neurodamus/node.py

@@ -861,9 +861,9 @@ def enable_modifications(self):
         log_stage("Enabling modifications...")
 
         mod_manager = ModificationManager(self._target_manager)
-        for name, mod_info in SimConfig.modifications.items():
-            target_spec = TargetSpec(mod_info["Target"], None)
-            logging.info(" * [MOD] %s: %s -> %s", name, mod_info["Type"], target_spec)
+        for mod_info in SimConfig.modifications:
+            target_spec = TargetSpec(mod_info.node_set, None)
+            logging.info(" * [MOD] %s: %s -> %s", mod_info.name, mod_info.type.name, target_spec)
             mod_manager.interpret(target_spec, mod_info)
 
     def write_and_get_population_offsets(self) -> tuple[dict, dict, dict]:

tests/scientific/test_v5_modification.py

@@ -1,12 +1,32 @@
 from pathlib import Path
+import pytest
 from neurodamus.utils.logging import log_verbose
 # !! NOTE: Please dont import Neuron or Nd objects. pytest will trigger Neuron instantiation!
 
-SIM_DIR = Path(__file__).parent.parent.absolute() / "simulations" / "v5_sonata"
-CONFIG_FILE_MINI = SIM_DIR / "simulation_config_mini.json"
-
-
-def test_TTX_modification():
+from tests.conftest import V5_SONATA
+
+@pytest.mark.parametrize(
+    "create_tmp_simulation_config_file",
+    [
+        {
+        "src_dir": str(V5_SONATA),
+        "simconfig_file": "simulation_config_mini.json",
+        "extra_config": {
+            "conditions": {
+                "modifications": [
+                    {
+                        "name": "applyTTX",
+                        "node_set": "Mini5",
+                        "type": "TTX"
+                    }
+                ]
+            },
+        }
+        }
+    ],
+    indirect=True,
+)
+def test_TTX_modification(create_tmp_simulation_config_file):
     """
     A test of enabling TTX with a short simulation.
     Expected outcome is non-zero spike count without TTX, zero with TTX.
@@ -18,7 +38,7 @@ def test_TTX_modification():
     from neurodamus.node import Node
 
     GlobalConfig.verbosity = LogLevel.VERBOSE
-    n = Node(str(CONFIG_FILE_MINI))
+    n = Node(create_tmp_simulation_config_file)
 
     # setup sim
     n.load_targets()
@@ -30,10 +50,6 @@ def test_TTX_modification():
     # _spike_vecs is a list of (spikes, ids)
     nspike_noTTX = sum(len(spikes) for spikes, _ in n._spike_vecs)
 
-    # append modification to config directly
-    TTX_mod = {"Type": "TTX", "Target": "Mini5"}
-    SimConfig.modifications["applyTTX"] = TTX_mod
-
     # setup sim again
     Nd.t = 0.0
     n._sim_ready = False
@@ -46,8 +62,29 @@ def test_TTX_modification():
     assert nspike_noTTX > 0
     assert nspike_TTX == 0
 
-
-def test_ConfigureAllSections_modification():
+@pytest.mark.parametrize(
+    "create_tmp_simulation_config_file",
+    [
+        {
+        "src_dir": str(V5_SONATA),
+        "simconfig_file": "simulation_config_mini.json",
+        "extra_config": {
+            "conditions": {
+                "modifications": [
+                    {
+                        "name": "no_SK_E2",
+                        "node_set": "Mini5",
+                        "type": "ConfigureAllSections",
+                        "section_configure": "%s.gSK_E2bar_SK_E2 = 0"
+                    }
+                ]
+            },
+        }
+        }
+    ],
+    indirect=True,
+)
+def test_ConfigureAllSections_modification(create_tmp_simulation_config_file):
     """
     A test of performing ConfigureAllSections with a short simulation.
     Expected outcome is higher spike count when enabled.
@@ -59,7 +96,7 @@ def test_ConfigureAllSections_modification():
     from neurodamus.node import Node
 
     GlobalConfig.verbosity = LogLevel.VERBOSE
-    n = Node(str(CONFIG_FILE_MINI))
+    n = Node(create_tmp_simulation_config_file)
 
     # setup sim
     n.load_targets()
@@ -70,12 +107,6 @@ def test_ConfigureAllSections_modification():
     n.solve(tstop=150)  # longer duration to see influence on spikes
     nspike_noConfigureAllSections = sum(len(spikes) for spikes, _ in n._spike_vecs)
 
-    # append modification to config directly
-    ConfigureAllSections_mod = {"Type": "ConfigureAllSections",
-                                "Target": "Mini5",
-                                "SectionConfigure": "%s.gSK_E2bar_SK_E2 = 0"}
-    SimConfig.modifications["no_SK_E2"] = ConfigureAllSections_mod
-
     # setup sim again
     Nd.t = 0.0
     n._sim_ready = False

tests/unit/test_modifications.py

@@ -1,16 +1,36 @@
 import numpy as np
 import pytest
 
-from tests.conftest import RINGTEST_DIR
-
-from neurodamus.core.configuration import ConfigurationError, SimConfig
+from neurodamus.core.configuration import ConfigurationError
 from neurodamus.modification_manager import ModificationManager
 from neurodamus.node import Neurodamus, Node
+from types import SimpleNamespace
 
-SIMULATION_CONFIG_FILE = RINGTEST_DIR / "simulation_config.json"
+from tests.conftest import RINGTEST_DIR
 
+SIMULATION_CONFIG_FILE = RINGTEST_DIR / "simulation_config.json"
 
-def test_applyTTX():
+@pytest.mark.parametrize(
+    "create_tmp_simulation_config_file",
+    [
+        {
+            "simconfig_fixture": "ringtest_baseconfig",
+            "extra_config": {
+                "conditions": {
+                    "modifications": [
+                        {
+                            "name": "applyTTX",
+                            "type": "TTX",
+                            "node_set": "RingA"
+                        }
+                    ]
+                },
+            },
+        }
+    ],
+    indirect=True,
+)
+def test_applyTTX(create_tmp_simulation_config_file):
     """
     A test of enabling TTX with a short simulation.
     As Ringtest cells don't contain mechanisms that use the TTX concentration
@@ -21,7 +41,7 @@ def test_applyTTX():
     # NeuronWrapper needs to be imported at function level
     from neurodamus.core import NeuronWrapper as Nd
 
-    n = Node(str(SIMULATION_CONFIG_FILE))
+    n = Node(create_tmp_simulation_config_file)
 
     # setup sim
     n.load_targets()
@@ -33,24 +53,29 @@ def test_applyTTX():
     for sec in cell.all:
         assert not Nd.ismembrane("TTXDynamicsSwitch", sec=sec)
 
-    # append modification to config directly
-    TTX_mod = {"Type": "TTX", "Target": "RingA"}
-    SimConfig.modifications["applyTTX"] = TTX_mod
-
     n.enable_modifications()
 
     # check TTXDynamicsSwitch is inserted after modifications
     for sec in cell.all:
         assert Nd.ismembrane("TTXDynamicsSwitch", sec=sec)
 
-
 @pytest.mark.parametrize(
     "create_tmp_simulation_config_file",
     [
         {
             "simconfig_fixture": "ringtest_baseconfig",
             "extra_config": {
                 "target_simulator": "NEURON",
+                "conditions": {
+                    "modifications": [
+                        {
+                            "name": "no_SK_E2",
+                            "node_set": "Mosaic",
+                            "type": "ConfigureAllSections",
+                            "section_configure": "%s.gnabar_hh = 0",
+                        }
+                    ]
+                },
                 "inputs": {
                     "pulse": {
                         "module": "pulse",
@@ -95,14 +120,6 @@ def test_ConfigureAllSections(create_tmp_simulation_config_file):
     for sec in cell.all:
         assert getattr(sec, sec_variable) > 0
 
-    # append modification to config directly
-    ConfigureAllSections_mod = {
-        "Type": "ConfigureAllSections",
-        "Target": "Mosaic",
-        "SectionConfigure": f"%s.{sec_variable} = 0",
-    }
-    SimConfig.modifications["no_SK_E2"] = ConfigureAllSections_mod
-
     n.enable_modifications()
 
     # check section variable value after modifications
@@ -158,6 +175,55 @@ def test_ConfigureAllSections_AugAssign(create_tmp_simulation_config_file):
     assert np.isclose(soma2.e_pas, -7)
 
 
+@pytest.mark.parametrize(
+    "create_tmp_simulation_config_file",
+    [
+        {
+            "simconfig_fixture": "ringtest_baseconfig",
+            "extra_config": {
+                "target_simulator": "NEURON",
+                "conditions": {
+                    "modifications": [
+                        {
+                            "name": "no_SK_E2",
+                            "node_set": "RingA:oneCell",
+                            "type": "ConfigureAllSections",
+                            "section_configure": "%s.e_pas *= 0.1",
+                        },
+                        {
+                            "name": "no_SK_E2",
+                            "node_set": "RingA:oneCell",
+                            "type": "ConfigureAllSections",
+                            "section_configure": "%s.gnabar_hh *= 11",
+                        }
+
+                    ]
+                },
+            },
+        }
+    ],
+    indirect=True,
+)
+def test_ConfigureAllSections_AugAssign_name_clash(create_tmp_simulation_config_file):
+    """This should produce the same results as test_ConfigureAllSections_AugAssign
+    
+    However, here we apply the same modification in 2 steps with modifications 
+    that have the same name. Their combined effect should be equivalent 
+    to the modification in test_ConfigureAllSections_AugAssign.
+    """
+
+    # NeuronWrapper needs to be imported at function level
+    from neurodamus.core import NeuronWrapper as Nd
+
+    Neurodamus(create_tmp_simulation_config_file)
+    soma1 = Nd._pc.gid2cell(0).soma[0]
+    soma2 = Nd._pc.gid2cell(1).soma[0]
+
+    assert np.isclose(soma1.gnabar_hh, 0.12)
+    assert np.isclose(soma1.e_pas, -70)
+    assert np.isclose(soma2.gnabar_hh, 1.32)
+    assert np.isclose(soma2.e_pas, -7)
+
 @pytest.mark.parametrize(
     "create_tmp_simulation_config_file",
     [
@@ -193,6 +259,17 @@ def test_error_unknown_modification():
     with pytest.raises(ConfigurationError, match="Unknown Modification mod_blabla"):
         mod_manager.interpret(target_spec="dummy", mod_info={"Type": "mod_blabla"})
 
+def test_error_unknown_modification():
+    mod_manager = ModificationManager(target_manager="dummy")
+    unknown_type = object()
+    mod_info = SimpleNamespace(type=unknown_type)
+
+    with pytest.raises(
+        ConfigurationError,
+        match="Unknown Modification",
+    ):
+        mod_manager.interpret(target_spec="dummy", mod_info=mod_info)
+
 
 @pytest.mark.parametrize(
     "create_tmp_simulation_config_file",