Merge pull request #517 from FZJ-INM1-BDA/maint_find_regions

Maint: parc.find_regions as module method. clarify find_regions

siibra/__init__.py

@@ -51,7 +51,7 @@
 # forward access to some functions
 set_ebrains_token = _EbrainsRequest.set_token
 fetch_ebrains_token = _EbrainsRequest.fetch_token
-find_regions = _parcellation.Parcellation.find_regions
+find_regions = _parcellation.find_regions
 from_json = factory.Factory.from_json
 
 

siibra/core/atlas.py

@@ -197,37 +197,43 @@ def get_voi(self, space: _space.Space, point1: tuple, point2: tuple):
 
     def find_regions(
         self,
-        regionspec,
-        all_versions=False,
-        filter_children=True,
-        **kwargs
+        regionspec: str,
+        all_versions: bool = False,
+        filter_children: bool = True,
+        find_topmost: bool = False
     ):
         """
-        Find regions with the given specification in all
-        parcellations offered by the atlas. Additional kwargs
-        are passed on to Parcellation.find().
+        Find regions with the given specification in all parcellations offered
+        by the atlas.
 
         Parameters
         ----------
-        regionspec: str, regex, int, Region, MapIndex
+        regionspec: str, regex
             - a string with a possibly inexact name (matched both against the name and the identifier key)
             - a string in '/pattern/flags' format to use regex search (acceptable flags: aiLmsux, see at https://docs.python.org/3/library/re.html#flags)
             - a regex applied to region names
-            - a Region object
         all_versions : Bool, default: False
             If True, matched regions for all versions of a parcellation are returned.
         filter_children : bool, default: True
             If False, children of matched parents will be returned.
+        find_topmost : bool, default: False
+            If True (requires `filter_children=True`), will return parent
+            structures if all children are matched, even though the parent
+            itself might not match the specification.
 
         Returns
         -------
         list[Region]
             list of regions matching to the regionspec
         """
         result = []
-        for p in self._parcellation_ids:
-            parcobj = _parcellation.Parcellation.get_instance(p)
-            if parcobj.is_newest_version or all_versions:
-                match = parcobj.find(regionspec, filter_children=filter_children, **kwargs)
-                result.extend(match)
+        for p in self.parcellations:
+            if p.is_newest_version or all_versions:
+                result.extend(
+                    p.find(
+                        regionspec=regionspec,
+                        filter_children=filter_children,
+                        find_topmost=find_topmost
+                    )
+                )
         return result

siibra/core/parcellation.py

@@ -19,8 +19,9 @@
 from ..volumes import parcellationmap
 from ..exceptions import NoMapMatchingValues
 
+from functools import lru_cache
 import re
-from typing import Union, List, Dict, TYPE_CHECKING
+from typing import Union, List, TYPE_CHECKING
 try:
     from typing import Literal
 except ImportError:
@@ -77,8 +78,6 @@ def __lt__(self, other: 'ParcellationVersion'):
 
 class Parcellation(region.Region, configuration_folder="parcellations"):
 
-    _CACHED_REGION_SEARCHES: Dict[str, List[region.Region]] = {}
-
     def __init__(
         self,
         identifier: str,
@@ -209,43 +208,6 @@ def get_map(
             return spec_candidates[0]
         return candidates[0]
 
-    @staticmethod
-    def find_regions(region_spec: str, parents_only=True):
-        """
-        Find regions that match the given region specification in the subtree
-        headed by each parcellation in the registry.
-        Note
-        ----
-        Use Region.find() to search for a region in an instance of a
-        parcellation.
-
-        Parameters
-        ----------
-        regionspec: str
-            a string with a possibly inexact name, which is matched both
-            against the name and the identifier key,
-        parents_only: bool
-            If true, children of matched parents will not be returned
-        Returns
-        -------
-        List[Region]
-            list of matching regions
-        """
-        MEM = Parcellation._CACHED_REGION_SEARCHES
-        if region_spec not in MEM:
-            MEM[region_spec] = [
-                r
-                for p in Parcellation.registry()
-                for r in p.find(regionspec=region_spec)
-            ]
-        if parents_only:
-            return [
-                r for r in MEM[region_spec]
-                if (r.parent is None) or (r.parent not in MEM[region_spec])
-            ]
-        else:
-            return MEM[region_spec]
-
     @property
     def is_newest_version(self):
         return (self.version is None) or (self.version.next_id is None)
@@ -382,3 +344,44 @@ def __lt__(self, other):
             )
             return self.name < other.name
         return self.version.__lt__(other.version)
+
+
+@lru_cache(maxsize=128)
+def find_regions(
+    regionspec: str,
+    filter_children=True,
+    find_topmost=False
+):
+    """
+    Find regions matching the given region specification across all parcellation
+    instances in the registery.
+
+    Parameters
+    ----------
+    regionspec: str, regex, Region
+        - a string with a possibly inexact name (matched both against the name and the identifier key)
+        - a string in '/pattern/flags' format to use regex search (acceptable flags: aiLmsux) (see https://docs.python.org/3/library/re.html#flags)
+        - a regex applied to region names
+        - a Region object
+    filter_children : bool, default: True
+        If True, children of matched parents will not be returned
+    find_topmost : bool, default: False
+        If True (requires `filter_children=True`), will return parent
+        structures if all children are matched, even though the parent
+        itself might not match the specification.
+
+    Returns
+    -------
+    list[Region]
+        list of regions matching to the regionspec
+    """
+    result = []
+    for p in Parcellation.registry():
+        result.extend(
+            p.find(
+                regionspec=regionspec,
+                filter_children=filter_children,
+                find_topmost=find_topmost
+            )
+        )
+    return result

siibra/core/region.py

@@ -37,6 +37,7 @@
 import json
 from functools import wraps, reduce
 from concurrent.futures import ThreadPoolExecutor
+from functools import lru_cache
 
 
 REGEX_TYPE = type(re.compile("test"))
@@ -119,7 +120,7 @@ def __init__(
         )
         self._supported_spaces = None  # computed on 1st call of self.supported_spaces
         self._str_aliases = None
-        self._CACHED_REGION_SEARCHES = {}
+        self.find = lru_cache(maxsize=3)(self.find)
 
     def get_related_regions(self) -> Iterable["RegionRelationAssessments"]:
         """
@@ -262,7 +263,7 @@ def find(
 
         Parameters
         ----------
-        regionspec: str, regex, int, Region
+        regionspec: str, regex, Region
             - a string with a possibly inexact name (matched both against the name and the identifier key)
             - a string in '/pattern/flags' format to use regex search (acceptable flags: aiLmsux, see at https://docs.python.org/3/library/re.html#flags)
             - a regex applied to region names
@@ -283,11 +284,6 @@ def find(
         ---
         See example 01-003, find regions.
         """
-        key = (regionspec, filter_children, find_topmost)
-        MEM = self._CACHED_REGION_SEARCHES
-        if key in MEM:
-            return MEM[key]
-
         if isinstance(regionspec, str):
             # convert the specified string into a regex for matching
             regex_match = self._regex_re.match(regionspec)
@@ -349,7 +345,7 @@ def find(
         found_regions = sorted(set(candidates), key=lambda r: r.depth)
 
         # reverse is set to True, since SequenceMatcher().ratio(), higher == better
-        MEM[key] = (
+        return (
             sorted(
                 found_regions,
                 reverse=True,
@@ -358,8 +354,6 @@ def find(
             if isinstance(regionspec, str) else found_regions
         )
 
-        return MEM[key]
-
     def matches(self, regionspec):
         """
         Checks whether this region matches the given region specification.

siibra/features/anchor.py

@@ -19,7 +19,7 @@
 from ..core.structure import BrainStructure
 from ..core.assignment import AnatomicalAssignment, Qualification
 from ..locations.location import Location
-from ..core.parcellation import Parcellation
+from ..core.parcellation import Parcellation, find_regions
 from ..core.region import Region
 from ..core.space import Space
 from ..exceptions import SpaceWarpingFailedError
@@ -126,13 +126,13 @@ def regions(self) -> Dict[Region, Qualification]:
             # decode the region specification into a dict of region objects and assignment qualifications
             regions = {
                 region: Qualification.EXACT
-                for region in Parcellation.find_regions(self._regionspec)
+                for region in find_regions(self._regionspec, filter_children=True, find_topmost=False)
                 if region.species in self.species
             }
             # add more regions from possible aliases of the region spec
             for alt_species, aliases in self.region_aliases.items():
                 for alias_regionspec, qualificationspec in aliases.items():
-                    for r in Parcellation.find_regions(alias_regionspec):
+                    for r in find_regions(alias_regionspec, filter_children=True, find_topmost=False):
                         if r.species != alt_species:
                             continue
                         if r not in regions:

test/core/test_atlas.py

@@ -1,5 +1,5 @@
 import unittest
-from unittest.mock import patch, PropertyMock, MagicMock, call
+from unittest.mock import patch, PropertyMock, MagicMock
 from parameterized import parameterized
 
 import siibra
@@ -229,34 +229,34 @@ def test_get_voi(self, space_arg, mock_space_fn_name, arg, kwarg):
 
     @parameterized.expand(
         product(
-            ["str-input", 3, Region("hello world")], product([True, False], repeat=3)
+            ["str-input", Region("hello world")], product([True, False], repeat=3)
         )
     )
     def test_find_regions(self, regionspec, bool_flags):
         all_versions, filter_children, _ = bool_flags
-        with patch.object(Parcellation, "get_instance") as get_instance_mock:
-            parc1 = MockParc(True)
+        find_topmost = False  # adds parents if children is matched but parent is not. works only if filter_children is True.
+        with patch.object(Atlas, "parcellations", new_callable=PropertyMock) as parcellations_mock:
+            parc1 = MockParc(is_newest_version=True)
             parc1.find.return_value = [MockObj(), MockObj(), MockObj()]
-            parc2 = MockParc(False)
+            parc2 = MockParc(is_newest_version=False)
             parc2.find.return_value = [MockObj(), MockObj(), MockObj()]
-            parc3 = MockParc(True)
+            parc3 = MockParc(is_newest_version=True)
             parc3.find.return_value = [MockObj(), MockObj(), MockObj()]
-            parc4 = MockParc(True)
+            parc4 = MockParc(is_newest_version=True)
             parc4.find.return_value = []
 
-            get_instance_mock.side_effect = [parc1, parc2, parc3, *repeat(parc4, 35)]
+            parcellations_mock.return_value = [parc1, parc2, parc3, *repeat(parc4, 35)]
 
             actual_result = self.atlas.find_regions(
                 regionspec, all_versions, filter_children
             )
 
-            get_instance_mock.assert_has_calls(
-                [call(pid) for pid in human_atlas_json.get("parcellations")]
-            )
+            parcellations_mock.assert_called_once()
+
             for p in [parc1, parc2, parc3]:
                 if all_versions or p.is_newest_version:
                     p.find.assert_called_once_with(
-                        regionspec, filter_children=filter_children
+                        regionspec=regionspec, filter_children=filter_children, find_topmost=find_topmost
                     )
 
             self.assertTrue(isinstance(p, MockObj) for p in actual_result)

test/core/test_parcellation.py

@@ -1,4 +1,7 @@
 import unittest
+from siibra.core.parcellation import Parcellation, ParcellationVersion, MapType, find_regions
+from siibra.core.region import Region
+from siibra.commons import Species, MapIndex
 from uuid import uuid4
 from parameterized import parameterized
 from unittest.mock import patch, MagicMock
@@ -40,6 +43,15 @@ def __init__(self, children) -> None:
         self.find = MagicMock()
 
 
+class DummyRegion:
+    def __init__(self, root, children) -> None:
+        self.children = children
+        self.root = root
+        for c in children:
+            c.parent = self
+        self.find = MagicMock()
+
+
 class DummyMap:
     def __init__(
         self, space_returns=True, parcellation_returns=True, maptype=MapType.LABELLED
@@ -203,27 +215,43 @@ def test_get_map(
 
     @parameterized.expand(
         [
-            (True,),
-            (False,),
+            (True, ),
+            (False, ),
         ]
     )
-    def test_find_regions(self, parents_only):
-        Parcellation._CACHED_REGION_SEARCHES = {}
+    def test_find_regions(self, filter_children):
+        find_topmost = False  # adds parents if children is matched but parent is not. works only if filter_children is True.
         with patch.object(Parcellation, "registry") as parcellation_registry_mock:
             parc1 = DummyParcellation([])
             parc2 = DummyParcellation([])
-            parc3 = DummyParcellation([parc1, parc2])
+            parc3 = DummyParcellation([])
+            parc3.children = [
+                DummyRegion(parc3, []),
+                DummyRegion(parc3, []),
+                DummyRegion(parc3, []),
+                DummyRegion(parc3, []),
+            ]
 
-            for p in [parc1, parc2, parc3]:
-                p.find.return_value = [p]
             parcellation_registry_mock.return_value = [parc1, parc2, parc3]
 
-            result = Parcellation.find_regions("fooz", parents_only)
+            for p in [parc1, parc2, parc3]:
+                if filter_children:
+                    p.find.return_value = [p]
+                else:
+                    p.find.return_value = [p] + p.children
+
+            result = find_regions("fooz", filter_children, find_topmost)
 
             parcellation_registry_mock.assert_called_once()
             for p in [parc1, parc2, parc3]:
-                p.find.assert_called_once_with(regionspec="fooz")
-            self.assertEqual(result, [parc3] if parents_only else [parc1, parc2, parc3])
+                p.find.assert_called_once_with(
+                    regionspec="fooz",
+                    filter_children=filter_children,
+                    find_topmost=find_topmost
+                )
+
+            expected_result = [parc1, parc2, parc3] if filter_children else [parc1, parc2, parc3] + parc3.children
+            self.assertEqual(result, expected_result)
 
     @parameterized.expand([
         # partial matches work
@@ -239,7 +267,6 @@ def test_get_region(self, regionspec, find_topmost, allow_tuple, result):
         self.parc.children = [region_parent]
         self.assertIs(self.parc.get_region(regionspec, find_topmost, allow_tuple), result)
 
-
 @pytest.mark.parametrize('space_id,parc_id,map_type', [
     ('waxholm', 'waxholm v4', 'labelled')
 ])