Extend circuit node_sets with simulation node sets (#235)

* Add an update method to NodeSets.py

* Make NodeSets.content a property

* added tests, as well as the scheme on simulation node_sets

* Fix: pycodestyle raises "line too long" on _version.py.

The file should not even be checked.
Disabled line length check on pycodestyle: it is handled by other linters.

* restrict libsonata>=0.1.24

* omit _version.py in coverage

* update changelog

* restrict pylint<3.0.0, until the cyclic-import issue is resolved

* Fixed according to review

CHANGELOG.rst

@@ -11,7 +11,10 @@ New Features
 Improvements
 ~~~~~~~~~~~~
 - Node set resolution is done by libsonata
-- Added kwarg: `raise_missing_property` to `NodePopulation.get`
+- Simulation node set extends Circuit node set
+
+  - A warning is raised if any of the circuit's node sets is overwritten
+- Added kwarg: ``raise_missing_property`` to ``NodePopulation.get``
 - Undeprecated calling ``Edges.get`` and ``EdgePopulation.get`` with ``properties=None``
 
 Breaking Changes

bluepysnap/node_sets.py

@@ -21,6 +21,7 @@
 https://github.com/AllenInstitute/sonata/blob/master/docs/SONATA_DEVELOPER_GUIDE.md#node-sets-file
 """
 
+import copy
 import json
 
 import libsonata
@@ -62,12 +63,13 @@ def __init__(self, content, instance):
         """Initializes a node set object from a node sets file.
 
         Args:
-            filepath (str/Path): Path to a SONATA node sets file.
+            content (dict): Node sets as a dictionary.
+            instance (libsonata.NodeSets): ``libsonata`` node sets instance.
 
         Returns:
             NodeSets: A NodeSets object.
         """
-        self.content = content
+        self._content = content
         self._instance = instance
 
     @classmethod
@@ -89,6 +91,35 @@ def from_dict(cls, content):
         """Create NodeSets instance from a dict."""
         return cls.from_string(json.dumps(content))
 
+    @property
+    def content(self):
+        """Access (a copy of) the node sets contents."""
+        return copy.deepcopy(self._content)
+
+    @property
+    def to_libsonata(self):
+        """Libsonata instance of the NodeSets."""
+        return self._instance
+
+    def update(self, node_sets):
+        """Update the contents of the node set.
+
+        Args:
+            node_sets (bluepysnap.NodeSets): The node set to extend this node set with.
+
+        Returns:
+            set: Names of any overwritten node sets.
+        """
+        if isinstance(node_sets, NodeSets):
+            overwritten = self._instance.update(node_sets.to_libsonata)
+            self._content.update(node_sets.content)
+            return overwritten
+
+        raise BluepySnapError(
+            f"Unexpected type: '{type(node_sets).__name__}' "
+            f"(expected: '{self.__class__.__name__}')"
+        )
+
     def __contains__(self, name):
         """Check if node set exists."""
         if isinstance(name, str):

bluepysnap/simulation.py

@@ -16,6 +16,7 @@
 # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 """Simulation access."""
 
+import warnings
 from pathlib import Path
 
 from cached_property import cached_property
@@ -46,6 +47,16 @@ def _collect_frame_reports(sim):
     return res
 
 
+def _warn_on_overwritten_node_sets(overwritten, print_max=10):
+    """Helper function to warn and print overwritten nodesets."""
+    if (n := len(overwritten)) > 0:
+        names = ", ".join(list(overwritten)[:print_max]) + (", ..." if n > print_max else "")
+        warnings.warn(
+            f"Simulation node sets overwrite {n} node set(s) in Circuit node sets: {names}",
+            RuntimeWarning,
+        )
+
+
 class Simulation:
     """Access to Simulation data."""
 
@@ -125,8 +136,16 @@ def simulator(self):
     @cached_property
     def node_sets(self):
         """Returns the NodeSets object bound to the simulation."""
-        path = self.to_libsonata.node_sets_file
-        return NodeSets.from_file(path) if path else NodeSets.from_dict({})
+        try:
+            node_sets = NodeSets.from_file(self.circuit.to_libsonata.node_sets_path)
+        except BluepySnapError:  # Error raised if circuit can not be instantiated
+            node_sets = NodeSets.from_dict({})
+
+        if path := self.to_libsonata.node_sets_file:
+            overwritten = node_sets.update(NodeSets.from_file(path))
+            _warn_on_overwritten_node_sets(overwritten)
+
+        return node_sets
 
     @cached_property
     def spikes(self):

pyproject.toml

@@ -13,6 +13,10 @@ known_local_folder  =  [
 'test_module',
 ]
 
+[tool.coverage.run]
+omit = [
+    "bluepysnap/_version.py",
+]
 [tool.pylint.main]
 # Files or directories to be skipped. They should be base names, not paths.
 ignore = ["CVS", "_version.py"]

setup.py

@@ -48,7 +48,7 @@ def __init__(self, *args, **kwargs):
         "importlib_resources>=5.0.0",
         "jsonschema>=4.18.0,<5.0.0",
         "referencing>=0.28.4",
-        "libsonata>=0.1.21,<1.0.0",
+        "libsonata>=0.1.24,<1.0.0",
         "morphio>=3.0.0,<4.0.0",
         "morph-tool>=2.4.3,<3.0.0",
         "numpy>=1.8",

tests/test_node_sets.py

@@ -1,4 +1,5 @@
 import json
+import re
 from unittest.mock import patch
 
 import libsonata
@@ -57,6 +58,29 @@ def test_from_dict(self):
         res = test_module.NodeSets.from_dict(self.test_obj.content)
         assert res.content == self.test_obj.content
 
+    def test_update(self):
+        # update all keys
+        tested = test_module.NodeSets.from_file(str(TEST_DATA_DIR / "node_sets_file.json"))
+        res = tested.update(tested)
+
+        # should return all keys as replaced
+        assert res == {*self.test_obj}
+        assert tested.content == self.test_obj.content
+
+        # actually add a new node set
+        additional = {"test": {"test_property": ["test_value"]}}
+        res = tested.update(test_module.NodeSets.from_dict(additional))
+        expected_content = {**self.test_obj.content, **additional}
+
+        # None of the keys should be replaced
+        assert res == set()
+        assert tested.content == expected_content
+
+        with pytest.raises(
+            BluepySnapError, match=re.escape("Unexpected type: 'str' (expected: 'NodeSets')")
+        ):
+            tested.update("")
+
     def test_contains(self):
         assert "Layer23" in self.test_obj
         assert "find_me_you_will_not" not in self.test_obj

tests/test_simulation.py

@@ -1,6 +1,7 @@
 import json
 import os
 import pickle
+import warnings
 
 import libsonata
 import pytest
@@ -20,6 +21,21 @@
 from utils import PICKLED_SIZE_ADJUSTMENT, TEST_DATA_DIR, copy_test_data, edit_config
 
 
+def test__warn_on_overwritten_node_sets():
+    test_module._warn_on_overwritten_node_sets
+
+    # No warnings if no overwritten
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        test_module._warn_on_overwritten_node_sets(set())
+
+    with pytest.warns(RuntimeWarning, match="Simulation node sets overwrite 3 .*: a, b, c"):
+        test_module._warn_on_overwritten_node_sets(["a", "b", "c"])
+
+    with pytest.warns(RuntimeWarning, match=r"Simulation node sets overwrite 3 .*: a, b, \.\.\."):
+        test_module._warn_on_overwritten_node_sets(["a", "b", "c"], print_max=2)
+
+
 def test_all():
     simulation = test_module.Simulation(str(TEST_DATA_DIR / "simulation_config.json"))
     assert simulation.config["network"] == str(TEST_DATA_DIR / "circuit_config.json")
@@ -41,8 +57,15 @@ def test_all():
     assert simulation.conditions.celsius == 34.0
     assert simulation.conditions.v_init == -80
 
-    assert isinstance(simulation.node_sets, NodeSets)
-    assert simulation.node_sets.content == {"Layer23": {"layer": [2, 3]}}
+    with pytest.warns(RuntimeWarning, match="Simulation node sets overwrite 1 .* Layer23"):
+        assert isinstance(simulation.node_sets, NodeSets)
+
+    expected_content = {
+        **json.loads((TEST_DATA_DIR / "node_sets.json").read_text()),
+        "Layer23": {"layer": [2, 3]},
+    }
+    assert simulation.node_sets.content == expected_content
+
     assert isinstance(simulation.spikes, SpikeReport)
     assert isinstance(simulation.spikes["default"], PopulationSpikeReport)
 

tox.ini

@@ -28,7 +28,7 @@ deps =
     isort
     pycodestyle
     pydocstyle
-    pylint
+    pylint<3.0.0  # overzealous cyclic-import in pylint>=3.0.0, see: https://github.com/pylint-dev/pylint/issues/9124
 commands =
     black --check .
     isort --check {[base]name} tests setup.py doc/source/conf.py
@@ -53,12 +53,12 @@ commands =
 allowlist_externals = make
 
 # E203: whitespace before ':'
+# E501: line too long (handled by black)
 # E731: do not assign a lambda expression, use a def
 # W503: line break after binary operator
 # W504: line break before binary operator
 [pycodestyle]
-ignore = E203,E731,W503,W504
-max-line-length = 100
+ignore = E203,E501,E731,W503,W504
 
 [pydocstyle]
 # ignore the following