From 175bed30f43ce1b95301ee6ab00eb89422cfb3e1 Mon Sep 17 00:00:00 2001 From: Ahmet Nihat Simsek Date: Wed, 13 Nov 2024 13:09:04 +0100 Subject: [PATCH 1/4] Raise if `get_map` can't find any maps instead of logs --- siibra/core/parcellation.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/siibra/core/parcellation.py b/siibra/core/parcellation.py index b5fa50148..ca835868c 100644 --- a/siibra/core/parcellation.py +++ b/siibra/core/parcellation.py @@ -18,10 +18,13 @@ from ..commons import logger, MapType, Species from ..volumes import parcellationmap -from typing import Union, List, Dict +from typing import Union, List, Dict, TYPE_CHECKING import re +if TYPE_CHECKING: + from .space import Space + # NOTE : such code could be used to automatically resolve # multiple matching parcellations for a short spec to the newset version: # try: @@ -140,7 +143,12 @@ def matches(self, spec): self._CACHED_MATCHES[spec] = True return super().matches(spec) - def get_map(self, space=None, maptype: Union[str, MapType] = MapType.LABELLED, spec: str = ""): + def get_map( + self, + space: Union[str, "Space"], + maptype: Union[str, MapType] = MapType.LABELLED, + spec: str = "" + ): """ Get the maps for the parcellation in the requested template space. @@ -153,8 +161,8 @@ def get_map(self, space=None, maptype: Union[str, MapType] = MapType.LABELLED, s Parameters ---------- space: Space or str - template space specification - maptype: MapType + reference space specification such as name, id, or a `Space` instance. + maptype: MapType or str Type of map requested (e.g., statistical or labelled). Use MapType.STATISTICAL to request probability maps. Defaults to MapType.LABELLED. @@ -176,19 +184,16 @@ def get_map(self, space=None, maptype: Union[str, MapType] = MapType.LABELLED, s m for m in parcellationmap.Map.registry() if m.space.matches(space) and m.maptype == maptype - and m.parcellation and m.parcellation.matches(self) ] if len(candidates) == 0: - logger.error(f"No {maptype} map in {space} available for {str(self)}") - return None + raise ValueError(f"No '{maptype}' map in '{space}' available for {str(self)}") if len(candidates) > 1: spec_candidates = [ c for c in candidates if all(w.lower() in c.name.lower() for w in spec.split()) ] if len(spec_candidates) == 0: - logger.warning(f"'{spec}' does not match any options from {[c.name for c in candidates]}.") - return None + raise ValueError(f"'{spec}' does not match any options from {[c.name for c in candidates]}.") if len(spec_candidates) > 1: logger.warning( f"Multiple maps are available in this specification of space, parcellation, and map type.\n" From 5a2973481ab96dc2d86205d0820981578c2e4255 Mon Sep 17 00:00:00 2001 From: Ahmet Nihat Simsek Date: Thu, 14 Nov 2024 10:21:21 +0100 Subject: [PATCH 2/4] Use specific exception `NoMapMatchingValues` for `get_map` --- siibra/core/parcellation.py | 12 +++++++----- siibra/exceptions.py | 4 ++++ test/core/test_parcellation.py | 21 ++++++++++++--------- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/siibra/core/parcellation.py b/siibra/core/parcellation.py index ca835868c..f791d944a 100644 --- a/siibra/core/parcellation.py +++ b/siibra/core/parcellation.py @@ -17,8 +17,9 @@ from ..commons import logger, MapType, Species from ..volumes import parcellationmap +from ..exceptions import NoMapMatchingValues -from typing import Union, List, Dict, TYPE_CHECKING +from typing import Union, List, Dict, TYPE_CHECKING, Literal import re @@ -146,7 +147,7 @@ def matches(self, spec): def get_map( self, space: Union[str, "Space"], - maptype: Union[str, MapType] = MapType.LABELLED, + maptype: Union[Literal['labelled', 'statistical'], MapType] = MapType.LABELLED, spec: str = "" ): """ @@ -177,8 +178,9 @@ def get_map( A ParcellationMap representing the volumetric map or a SparseMap representing the list of statistical maps. """ - if not isinstance(maptype, MapType): + if isinstance(maptype, str): maptype = MapType[maptype.upper()] + assert isinstance(maptype, MapType), "Possible values of `maptype` are `MapType`s, 'labelled', 'statistical'." candidates = [ m for m in parcellationmap.Map.registry() @@ -187,13 +189,13 @@ def get_map( and m.parcellation.matches(self) ] if len(candidates) == 0: - raise ValueError(f"No '{maptype}' map in '{space}' available for {str(self)}") + raise NoMapMatchingValues(f"No '{maptype}' map in '{space}' available for {str(self)}") if len(candidates) > 1: spec_candidates = [ c for c in candidates if all(w.lower() in c.name.lower() for w in spec.split()) ] if len(spec_candidates) == 0: - raise ValueError(f"'{spec}' does not match any options from {[c.name for c in candidates]}.") + raise NoMapMatchingValues(f"'{spec}' does not match any options from {[c.name for c in candidates]}.") if len(spec_candidates) > 1: logger.warning( f"Multiple maps are available in this specification of space, parcellation, and map type.\n" diff --git a/siibra/exceptions.py b/siibra/exceptions.py index 17dbed61e..8ede3abf4 100644 --- a/siibra/exceptions.py +++ b/siibra/exceptions.py @@ -53,3 +53,7 @@ class ZeroVolumeBoundingBox(Exception): class NoneCoordinateSuppliedError(ValueError): pass + + +class NoMapMatchingValues(ValueError): + pass diff --git a/test/core/test_parcellation.py b/test/core/test_parcellation.py index e24f7b9dc..0faf0013a 100644 --- a/test/core/test_parcellation.py +++ b/test/core/test_parcellation.py @@ -11,6 +11,7 @@ from siibra.core.region import Region from siibra.commons import Species import siibra +from siibra.exceptions import NoMapMatchingValues correct_json = { "name": "foobar", @@ -82,7 +83,7 @@ def test_attr(self): INCORRECT_MAP_TYPE = "incorrect_type" -class InputMap(NamedTuple): +class InputMapType(NamedTuple): input: Union[str, MapType] alias: MapType @@ -121,10 +122,11 @@ def setUpClass(cls) -> None: [None, "space arg", DummySpace()], # input, maptype [ - InputMap(None, MapType.LABELLED), - InputMap("labelled", MapType.LABELLED), - InputMap(MapType.LABELLED, MapType.LABELLED), - InputMap(INCORRECT_MAP_TYPE, None), + InputMapType(None, None), + InputMapType("labelled", MapType.LABELLED), + InputMapType("Laballlled", None), + InputMapType(MapType.LABELLED, MapType.LABELLED), + InputMapType(INCORRECT_MAP_TYPE, None), ], # volume configuration product( @@ -148,12 +150,11 @@ def setUpClass(cls) -> None: ) ) def test_get_map( - self, space, maptype_input: InputMap, vol_spec: Tuple[MapConfig, MapConfig] + self, space, maptype_input: InputMapType, vol_spec: Tuple[MapConfig, MapConfig] ): from siibra.volumes import Map ExpectedException = None - expected_return_idx = None for idx, vol_s in enumerate(vol_spec): if ( @@ -163,9 +164,11 @@ def test_get_map( ): expected_return_idx = idx break + else: + ExpectedException = NoMapMatchingValues if maptype_input.alias is None: - ExpectedException = KeyError + ExpectedException = AssertionError if maptype_input.input is None else KeyError with patch.object(Map, "registry") as map_registry_mock: registry_return = [ @@ -196,7 +199,7 @@ def test_get_map( if expected_return_idx is not None: self.assertIs(map, registry_return[expected_return_idx]) else: - self.assertIsNone(map) + self.assertRaises(ExpectedException) @parameterized.expand( [ From 01d950087117f5c39fa6710c48482589804762e7 Mon Sep 17 00:00:00 2001 From: Ahmet Nihat Simsek Date: Thu, 14 Nov 2024 11:04:19 +0100 Subject: [PATCH 3/4] test: do not run fast tests when full suite is running --- .github/workflows/siibra-testing.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/siibra-testing.yml b/.github/workflows/siibra-testing.yml index 5676e4a08..2407ac7a6 100644 --- a/.github/workflows/siibra-testing.yml +++ b/.github/workflows/siibra-testing.yml @@ -70,7 +70,7 @@ jobs: python-version: ["3.12", "3.11", "3.10", "3.9", "3.8", "3.7"] unit-tests-fast: - if: ${{ github.event_name != 'pull_request' }} + if: ${{ github.event.pull_request == null }} needs: "use-custom-cfg" uses: ./.github/workflows/_unittest.yaml with: @@ -92,7 +92,7 @@ jobs: python-version: ["3.12", "3.11", "3.10", "3.9", "3.8", "3.7"] e2e-tests-fast: - if: ${{ github.event_name != 'pull_request' }} + if: ${{ github.event.pull_request == null }} needs: "use-custom-cfg" uses: ./.github/workflows/_e2e.yaml with: From b36aa4b6794b6c65bcd2d2775c38764f75936e31 Mon Sep 17 00:00:00 2001 From: Ahmet Nihat Simsek Date: Thu, 14 Nov 2024 11:10:06 +0100 Subject: [PATCH 4/4] typehint support for python 3.7 --- siibra/core/parcellation.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/siibra/core/parcellation.py b/siibra/core/parcellation.py index f791d944a..c5e99ed22 100644 --- a/siibra/core/parcellation.py +++ b/siibra/core/parcellation.py @@ -19,8 +19,13 @@ from ..volumes import parcellationmap from ..exceptions import NoMapMatchingValues -from typing import Union, List, Dict, TYPE_CHECKING, Literal import re +from typing import Union, List, Dict, TYPE_CHECKING +try: + from typing import Literal +except ImportError: + # support python 3.7 + from typing_extensions import Literal if TYPE_CHECKING: