Address reviewer comments

by CodeRabiit

Author: jsnel

pyglotaran_extras/inspect/kinetic_scheme/_layout.py

@@ -33,7 +33,7 @@ class LayoutAlgorithm(str, Enum):
         Force-directed Fruchterman-Reingold layout. For complex cyclic
         schemes.
     MANUAL
-        User-supplied positions passed through unchanged.
+        User-supplied compartment positions with derived GS positioning.
     """
 
     HIERARCHICAL = "hierarchical"
@@ -94,9 +94,8 @@ def compute_layout(
         horizontal_spacing = 3.0 * DEFAULT_NODE_WIDTH
 
     if algorithm == LayoutAlgorithm.MANUAL:
-        return _manual_layout(graph, manual_positions)
-
-    if algorithm == LayoutAlgorithm.SPRING:
+        positions = _manual_layout(graph, manual_positions)
+    elif algorithm == LayoutAlgorithm.SPRING:
         positions = _spring_layout(
             graph,
             horizontal_spacing=horizontal_spacing,

tests/inspect/kinetic_scheme/test_k_matrix_parser.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 from copy import deepcopy
+from types import SimpleNamespace
 
 import pytest
 from glotaran.testing.simulated_data.parallel_spectral_decay import SCHEME as SCHEME_PAR
@@ -119,16 +120,23 @@ def test_invalid_megacomplex_raises_value_error(self) -> None:
         with pytest.raises(ValueError, match="not found"):
             extract_transitions("nonexistent_mc", SCHEME_SEQ.model, SCHEME_SEQ.parameters)
 
-    def test_non_decay_megacomplex_raises_type_error(self) -> None:
+    def test_non_decay_megacomplex_raises_type_error(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
         """Non-decay megacomplex without get_k_matrix raises TypeError."""
-        # Create a model that has a coherent-artifact megacomplex
-        # For now, we test with a mock approach
-        # The sequential model only has decay megacomplexes, so we test
-        # that valid ones don't raise
-        transitions = extract_transitions(
-            "megacomplex_sequential_decay", SCHEME_SEQ.model, SCHEME_SEQ.parameters
+        model = deepcopy(SCHEME_SEQ.model)
+        mc_label = "megacomplex_coherent_artifact"
+        model.megacomplex[mc_label] = SimpleNamespace(type="coherent-artifact")
+
+        # Simulate filling a non-decay megacomplex object that does not expose
+        # get_k_matrix, which should trigger the TypeError path.
+        monkeypatch.setattr(
+            "pyglotaran_extras.inspect.kinetic_scheme._k_matrix_parser.fill_item",
+            lambda *_args, **_kwargs: SimpleNamespace(),
         )
-        assert len(transitions) > 0
+
+        with pytest.raises(TypeError, match="does not support k-matrix extraction"):
+            extract_transitions(mc_label, model, SCHEME_SEQ.parameters)
 
 
 class TestExtractTransitionsFiltering:

tests/inspect/kinetic_scheme/test_layout.py

@@ -233,6 +233,30 @@ def test_passthrough(self) -> None:
         assert positions["B"] == (1.0, 1.0)
         assert positions["C"] == (2.0, 0.0)
 
+    def test_ground_state_below_parent(self) -> None:
+        """Manual layout should also position ground state nodes below parents."""
+        graph = _make_sequential_graph()
+        manual = {"A": (0.0, 2.0), "B": (1.0, 1.0), "C": (2.0, 0.0)}
+        positions = compute_layout(graph, LayoutAlgorithm.MANUAL, manual_positions=manual)
+
+        assert "GS1" in positions
+        assert positions["GS1"][0] == positions["C"][0]
+        assert positions["GS1"][1] < positions["C"][1]
+
+    def test_avoids_ground_state_arrow_overlap(self) -> None:
+        """Manual layout should nudge a node below a GS-decaying parent."""
+        graph = KineticGraph.from_transitions(
+            [
+                Transition("A", "B", 0.5, "rates.k_AB", False, "mc1"),
+                Transition("A", "GS1", 0.1, "rates.k_A", True, "mc1"),
+            ]
+        )
+        manual = {"A": (0.0, 1.0), "B": (0.0, 0.0)}
+        positions = compute_layout(graph, LayoutAlgorithm.MANUAL, manual_positions=manual)
+
+        assert positions["B"][0] > 0.0
+        assert positions["B"][1] == 0.0
+
     def test_missing_positions_raises(self) -> None:
         """Missing node positions should raise ValueError."""
         graph = _make_sequential_graph()