Fix infinite loop in mag calculation during anisotropic downsampling (#934)

* Fix infinite loop in mag calculation during anisotropic downsampling

* update changelog

* fix typing

* Adapt naming of mags.

---------

Co-authored-by: MichaelBuessemeyer <[email protected]>
Co-authored-by: markbader <[email protected]>

Author: 3 people

webknossos/Changelog.md

@@ -19,6 +19,7 @@ For upgrade instructions, please check the respective _Breaking Changes_ section
 ### Changed
 
 ### Fixed
+- Fixed an infinite loop in the mag calculation during anisotropic downsampling in situations where the target mag cannot possibly be reached while adhering to the anisotropic downsampling scheme. [#934](https://github.com/scalableminds/webknossos-libs/pull/934) 
 
 
 ## [0.13.3](https://github.com/scalableminds/webknossos-libs/releases/tag/v0.13.3) - 2023-08-08

webknossos/poetry.lock

@@ -90,6 +90,7 @@ pytest-custom-exit-code = "^0.3.0"
 pytest-recording = "^0.12.0"
 pytest-split = "^0.8.0"
 pytest-sugar = "^0.9.4"
+pytest-timeout = "^2.1.0"
 types-python-dateutil = "^0.1.6"
 # packages for examples:
 fastremap = "^1.13.3"

webknossos/pyproject.toml

@@ -1,5 +1,6 @@
 import warnings
 from pathlib import Path
+from typing import List, Optional, Tuple
 
 import numpy as np
 import pytest
@@ -174,14 +175,9 @@ def test_downsample_multi_channel(tmp_path: Path) -> None:
     assert np.all(target_buffer == joined_buffer)
 
 
-def test_anisotropic_mag_calculation() -> None:
-    # This test does not test the exact input of the user:
-    # If a user does not specify a max_mag, then a default is calculated.
-    # Therefore, max_mag=None is not covered in this test case.
-    # The same applies for `voxel_size`:
-    # This is either extracted from the properties or set to comply with a specific sampling mode.
-
-    mag_tests = [
+@pytest.mark.parametrize(
+    "voxel_size, finest_mag, coarsest_mag, scheme",
+    [
         # Anisotropic
         (
             (10.5, 10.5, 24),  # voxel_size
@@ -260,27 +256,68 @@ def test_anisotropic_mag_calculation() -> None:
         ),
         (None, (2, 2, 1), (8, 8, 4), [(2, 2, 1), (4, 4, 2), (8, 8, 4)]),
         (None, (2, 2, 1), (8, 8, 8), [(2, 2, 1), (4, 4, 2), (8, 8, 4)]),
-    ]
-
-    for i in range(len(mag_tests)):
-        voxel_size, from_max_name, max_mag_name, scheme = mag_tests[i]
-        sampling_scheme = [Mag(m) for m in scheme]
-        from_mag = Mag(from_max_name)
-        max_mag = Mag(max_mag_name)
-
-        assert sampling_scheme[1:] == calculate_mags_to_downsample(
-            from_mag, max_mag, None, voxel_size
-        ), f"The calculated downsampling scheme of the {i+1}-th test case is wrong."
-
-    for i in range(len(mag_tests)):
-        voxel_size, finest_mag_name, from_mag_name, scheme = mag_tests[i]
-        sampling_scheme = [Mag(m) for m in scheme]
-        from_mag = Mag(from_mag_name)
-        finest_mag = Mag(finest_mag_name)
-
-        assert list(reversed(sampling_scheme[:-1])) == calculate_mags_to_upsample(
-            from_mag, finest_mag, None, voxel_size
-        ), f"The calculated upsampling scheme of the {i+1}-th test case is wrong."
+    ],
+)
+def test_mag_calculation(
+    voxel_size: Optional[Tuple[float, float, float]],
+    finest_mag: Tuple[int, int, int],
+    coarsest_mag: Tuple[int, int, int],
+    scheme: List[Tuple[int, int, int]],
+) -> None:
+    # This test does not test the exact input of the user:
+    # If a user does not specify a max_mag, then a default is calculated.
+    # Therefore, max_mag=None is not covered in this test case.
+    # The same applies for `voxel_size`:
+    # This is either extracted from the properties or set to comply with a specific sampling mode.
+
+    sampling_scheme = [Mag(m) for m in scheme]
+
+    assert sampling_scheme[1:] == calculate_mags_to_downsample(
+        Mag(finest_mag), Mag(coarsest_mag), None, voxel_size
+    ), "The calculated downsampling scheme is wrong."
+
+    assert list(reversed(sampling_scheme[:-1])) == calculate_mags_to_upsample(
+        Mag(coarsest_mag), Mag(finest_mag), None, voxel_size
+    ), "The calculated upsampling scheme is wrong."
+
+
+@pytest.mark.parametrize(
+    "voxel_size, finest_mag, coarsest_mag",
+    [
+        # Anisotropic
+        (
+            (10.5, 10.5, 24),
+            (2, 2, 2),
+            (16, 16, 2),
+        ),
+        (
+            (10.5, 10.5, 24),  # voxel_size
+            (1, 1, 1),  # from_mag
+            (1, 1, 2),  # max_mag
+        ),
+        (
+            (10.5, 10.5, 24),
+            (1, 1, 1),
+            (4, 4, 16),
+        ),
+    ],
+)
+@pytest.mark.timeout(1)
+def test_invalid_mag_calculation(
+    voxel_size: Optional[Tuple[float, float, float]],
+    finest_mag: Tuple[int, int, int],
+    coarsest_mag: Tuple[int, int, int],
+) -> None:
+    # This test does not test the exact input of the user:
+    # If a user does not specify a max_mag, then a default is calculated.
+    # Therefore, max_mag=None is not covered in this test case.
+    # The same applies for `voxel_size`:
+    # This is either extracted from the properties or set to comply with a specific sampling mode.
+
+    with pytest.raises(RuntimeError):
+        calculate_mags_to_downsample(
+            Mag(finest_mag), Mag(coarsest_mag), None, voxel_size
+        )
 
 
 def test_default_max_mag() -> None:

webknossos/tests/dataset/test_downsampling.py

@@ -95,7 +95,7 @@ def calculate_mags_to_downsample(
                 )
                 # In case of isotropic resolution but anisotropic mag we need to ensure unique max dims.
                 # current mag: 4-4-2, voxel_size: 1,1,1 -> new_mag: 4-4-4, therefore we skip this entry.
-                if new_mag.max_dim == current_mag.max_dim:
+                if new_mag.max_dim == current_mag.max_dim and new_mag != current_mag:
                     current_mag = new_mag
                     continue
             if new_mag == current_mag: