fix: fixes in tests + renaming for readability

Author: ajkswamy

src/spatialdata/_core/transformation_manager/__init__.py

@@ -375,8 +375,8 @@ def get_existing_direct_transformations(
         with suppress_direct_internal_attribute_access_warning():
             transforms = []
             assert target_cs in self.graph[source_cs], TransformationNotFoundError(source_cs.name, target_cs.name)
-            for _edge_key, edge in self.graph[source_cs][target_cs]:
-                transform: BaseTransformation = edge[TRANSFORM_KEY]
+            for edge_data in self.graph[source_cs][target_cs].values():
+                transform: BaseTransformation = edge_data[TRANSFORM_KEY]
                 transforms.append(transform)
             return transforms
 
@@ -415,7 +415,7 @@ def remove_specific_transformation(
             )
             self.graph.remove_edge(source_cs, target_cs, key=expected_edge_key)
 
-    def remove_all_transformations(
+    def remove_all_transformations_between_coordinate_systems(
         self,
         source_cs: NgffCoordinateSystem,
         target_cs: NgffCoordinateSystem,
@@ -441,12 +441,13 @@ def remove_all_transformations(
         # also checks if source_cs and target_cs exist
         with suppress_direct_internal_attribute_access_warning():
             assert len(self.graph[source_cs][target_cs]), TransformationNotFoundError(source_cs.name, target_cs.name)
-            for edge_key, _edge in self.graph[source_cs][target_cs]:
+            for edge_key in list(self.graph[source_cs][target_cs].keys()):
+                # need to covert keys() to list to freeze it, else it will change during the following removal
                 self.graph.remove_edge(source_cs, target_cs, key=edge_key)
 
     def _get_transformation_sequences_from_path_after_disambiguation(
         self,
-        paths: Sequence[list[NgffCoordinateSystem]],
+        paths: list[list[NgffCoordinateSystem]],
         expected_intermediate_transformations: list[BaseTransformation] | None,
     ) -> list[list[BaseTransformation]]:
         """
@@ -471,11 +472,12 @@ def _get_transformation_sequences_from_path_after_disambiguation(
                 self._get_edge_key_from_transform(it) for it in expected_intermediate_transformations
             }
         all_sequences = []
-        for path in paths:
+        deduplicated_paths = list({repr(x): x for x in paths}.values())
+        for path in deduplicated_paths:
             sequence = []
             for i in range(len(path) - 1):
                 edge_data = self.graph[path[i]][path[i + 1]]
-                if len(edge_data) >= 1:
+                if len(edge_data) > 1:
                     # when there are multiple edges between a pair of coordinate systems in the path
                     intermediate_transformation_key_here = intermediate_transformation_edge_keys & set(edge_data.keys())
                     if len(intermediate_transformation_key_here) == 0:
@@ -486,7 +488,9 @@ def _get_transformation_sequences_from_path_after_disambiguation(
                     # choosing the first one arbitrarily
                     sequence.append(edge_data[edge_key_to_use][TRANSFORM_KEY])
                 else:
-                    sequence.append(edge_data[0][TRANSFORM_KEY])
+                    # Only one edge, no ambiguity
+                    edge_key = next(iter(edge_data.keys()))
+                    sequence.append(edge_data[edge_key][TRANSFORM_KEY])
             all_sequences.append(sequence)
         return all_sequences
 
@@ -528,7 +532,7 @@ def get_all_shortest_transformation_sequences(
         """
         with suppress_direct_internal_attribute_access_warning():
             try:
-                paths = list(nx.shortest_simple_paths(self.graph, source=source_cs, target=target_cs))
+                paths = list(nx.all_shortest_paths(self.graph, source=source_cs, target=target_cs))
 
             except nx.NetworkXNoPath as nxe:
                 raise TransformationPathNotFoundError(source_cs.name, target_cs.name) from nxe

tests/core/transformation_manager/conftest.py

@@ -62,7 +62,7 @@ def four_point_graph() -> tuple[list[NgffCoordinateSystem], list[Scale | Transla
 
 
 @pytest.fixture
-def five_point_graph() -> tuple[list[NgffCoordinateSystem], list[Scale | Translation]]:
+def five_point_graph() -> tuple[list[NgffCoordinateSystem], list[Scale | Translation | Affine]]:
     """Fixture providing a five-point graph with five coordinate systems and five transformations."""
 
     coordinate_systems = [

tests/core/transformation_manager/test_transformation_manager.py

@@ -192,7 +192,9 @@ def test_add_transformation(fully_connected_two_point_graph):
         tm.add_transformation(cs1, cs2, transform)
 
         assert tm.graph.has_edge(cs1, cs2)
-        assert tm.graph[cs1][cs2][0][TRANSFORM_KEY] == transform
+        # Get the first edge key and check the transformation
+        edge_key = tm._get_edge_key_from_transform(transform)
+        assert tm.graph[cs1][cs2][edge_key][TRANSFORM_KEY] == transform
 
 
 def test_add_transformation_nonexistent_cs(fully_connected_two_point_graph):
@@ -224,7 +226,7 @@ def test_get_existing_transformation(fully_connected_two_point_graph):
     tm.add_transformation(cs1, cs2, transform)
 
     retrieved = tm.get_existing_direct_transformations(cs1, cs2)
-    assert retrieved == transform
+    assert retrieved == [transform]
 
 
 def test_get_existing_transformation_nonexistent(fully_connected_two_point_graph):
@@ -241,8 +243,8 @@ def test_get_existing_transformation_nonexistent(fully_connected_two_point_graph
         tm.get_existing_direct_transformations(cs1, cs2)
 
 
-def test_remove_transformation(fully_connected_two_point_graph):
-    """Test removing a transformation."""
+def test_remove_all_transformations_between_coordinate_systems(fully_connected_two_point_graph):
+    """Test removing all transformations  between coordinate systems."""
     with suppress_direct_internal_attribute_access_warning():
         tm = TransformationManager()
         [cs1, cs2], [transform] = fully_connected_two_point_graph
@@ -251,12 +253,12 @@ def test_remove_transformation(fully_connected_two_point_graph):
 
         tm.add_transformation(cs1, cs2, transform)
 
-        tm.remove_all_transformations(cs1, cs2)
+        tm.remove_all_transformations_between_coordinate_systems(cs1, cs2)
         assert not tm.graph.has_edge(cs1, cs2)
 
 
-def test_remove_transformation_nonexistent(fully_connected_two_point_graph):
-    """Test that removing a non-existent transformation raises TransformationNotFoundError."""
+def test_remove_all_transformation_nonexistent(fully_connected_two_point_graph):
+    """Test that removing non-existent transformations between coordinate systems raises TransformationNotFoundError."""
     with suppress_direct_internal_attribute_access_warning():
         tm = TransformationManager()
         [cs1, cs2], _ = fully_connected_two_point_graph
@@ -265,7 +267,41 @@ def test_remove_transformation_nonexistent(fully_connected_two_point_graph):
         with pytest.raises(
             TransformationNotFoundError, match=f"Transformation from '{cs1.name}' to '{cs2.name}' not found"
         ):
-            tm.remove_all_transformations(cs1, cs2)
+            tm.remove_all_transformations_between_coordinate_systems(cs1, cs2)
+
+
+def test_remove_specific_transformation_between_coordinate_systems(five_point_graph):
+    """Test removing specific transformation between coordinate systems."""
+
+    with suppress_direct_internal_attribute_access_warning():
+        tm = TransformationManager()
+        [_cs1, _cs2, cs3, _cs4, cs5], [_transform1, _transform2, _transform3, transform4, transform5] = five_point_graph
+        tm.add_coordinate_system(cs3)
+        tm.add_coordinate_system(cs5)
+
+        tm.add_transformation(cs3, cs5, transform4)
+        tm.add_transformation(cs3, cs5, transform5)
+
+        tm.remove_specific_transformation(cs3, cs5, transform4)
+        assert not tm.graph.has_edge(cs3, cs5, key=tm._get_edge_key_from_transform(transform4))
+
+
+def test_remove_specific_transformation_between_coordinate_systems_non_existent(five_point_graph):
+    """
+    Test that removing non-existent specific transformation between coordinate systems raises
+    TransformationNotFoundError.
+    """
+
+    with suppress_direct_internal_attribute_access_warning():
+        tm = TransformationManager()
+        [_cs1, _cs2, cs3, _cs4, cs5], [_transform1, _transform2, _transform3, transform4, transform5] = five_point_graph
+        tm.add_coordinate_system(cs3)
+        tm.add_coordinate_system(cs5)
+
+        with pytest.raises(
+            TransformationNotFoundError, match=f"Transformation from '{cs3.name}' to '{cs5.name}' not found"
+        ):
+            tm.remove_specific_transformation(cs3, cs5, transform4)
 
 
 def test_get_all_shortest_transformation_sequences_direct(four_point_graph):
@@ -354,17 +390,16 @@ def test_get_all_shortest_transformation_sequences_multiple_paths_multiple_edges
     tm.add_transformation(cs2, cs3, transform2)
     tm.add_transformation(cs1, cs4, transform2)
     tm.add_transformation(cs4, cs3, transform1)
-    tm.add_transformation(cs1, cs3, transform3)
-    tm.add_transformation(cs4, cs5, transform4)
-    tm.add_transformation(cs4, cs5, transform5)
+    # tm.add_transformation(cs1, cs3, transform3)
+    tm.add_transformation(cs3, cs5, transform4)
+    tm.add_transformation(cs3, cs5, transform5)
 
     sequences = tm.get_all_shortest_transformation_sequences(
         cs1, cs5, expected_intermediate_transformations=[transform4]
     )
-    assert len(sequences) == 3
+    assert len(sequences) == 2
     assert [transform1, transform2, transform4] in sequences
     assert [transform2, transform1, transform4] in sequences
-    assert [transform3, transform4] in sequences
 
 
 def test_get_all_shortest_transformation_sequences_multiple_paths_multiple_edges_failure(five_point_graph):
@@ -382,11 +417,11 @@ def test_get_all_shortest_transformation_sequences_multiple_paths_multiple_edges
     tm.add_transformation(cs1, cs4, transform2)
     tm.add_transformation(cs4, cs3, transform1)
     tm.add_transformation(cs1, cs3, transform3)
-    tm.add_transformation(cs4, cs5, transform4)
-    tm.add_transformation(cs4, cs5, transform5)
+    tm.add_transformation(cs3, cs5, transform4)
+    tm.add_transformation(cs3, cs5, transform5)
 
     with pytest.raises(TransformationPathAmbiguousError, match="Transformation Path ambiguous"):
-        tm.get_all_shortest_transformation_sequences(cs1, cs5, expected_intermediate_transformations=[transform4])
+        tm.get_all_shortest_transformation_sequences(cs1, cs5)
 
 
 def test_get_all_transformation_sequences(four_point_graph):
@@ -446,8 +481,8 @@ def test_get_all_transformation_sequences_multiple_paths_multiple_edges_success(
     tm.add_transformation(cs1, cs4, transform2)
     tm.add_transformation(cs4, cs3, transform1)
     tm.add_transformation(cs1, cs3, transform3)
-    tm.add_transformation(cs4, cs5, transform4)
-    tm.add_transformation(cs4, cs5, transform5)
+    tm.add_transformation(cs3, cs5, transform4)
+    tm.add_transformation(cs3, cs5, transform5)
 
     sequences = tm.get_all_transformation_sequences(cs1, cs5, expected_intermediate_transformations=[transform4])
     assert len(sequences) == 3
@@ -471,11 +506,11 @@ def test_get_all_transformation_sequences_multiple_paths_multiple_edges_failure(
     tm.add_transformation(cs1, cs4, transform2)
     tm.add_transformation(cs4, cs3, transform1)
     tm.add_transformation(cs1, cs3, transform3)
-    tm.add_transformation(cs4, cs5, transform4)
-    tm.add_transformation(cs4, cs5, transform5)
+    tm.add_transformation(cs3, cs5, transform4)
+    tm.add_transformation(cs3, cs5, transform5)
 
     with pytest.raises(TransformationPathAmbiguousError, match="Transformation Path ambiguous"):
-        tm.get_all_transformation_sequences(cs1, cs5, expected_intermediate_transformations=[transform4])
+        tm.get_all_transformation_sequences(cs1, cs5)
 
 
 def test_get_all_transformation_sequences_no_path(four_point_graph):