From b0de3e748cd2566089ed0af0c936c64529e1d931 Mon Sep 17 00:00:00 2001
From: Samuel Pastva <sam.pastva@gmail.com>
Date: Fri, 19 Jul 2024 12:13:09 +0200
Subject: [PATCH] Fixes a minor bug in parity cycle detection with self-loops.

---
 .../signed_directed_graph/_cycle_detection.rs |  7 +++--
 .../_feedback_vertex_set.rs                   | 28 ++++++++++++++++++-
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/src/_impl_regulatory_graph/signed_directed_graph/_cycle_detection.rs b/src/_impl_regulatory_graph/signed_directed_graph/_cycle_detection.rs
index 6a111df..9b61eac 100644
--- a/src/_impl_regulatory_graph/signed_directed_graph/_cycle_detection.rs
+++ b/src/_impl_regulatory_graph/signed_directed_graph/_cycle_detection.rs
@@ -144,7 +144,7 @@ impl SdGraph {
         }
 
         if upper_bound == 1 {
-            // Only self-loop can have one edge.
+            // Only a self-loop can have one edge.
             return None;
         }
 
@@ -158,7 +158,10 @@ impl SdGraph {
         // Direct predecessors can reach pivot in zero steps.
         let mut one_step = HashMap::with_hasher(FxBuildHasher::default());
         for (pred, sign) in &self.predecessors[pivot.to_index()] {
-            if restriction.contains(pred) {
+            // Pivot should never be inserted here (see also similar condition below), because
+            // it is already "on the path". Adding it again would mean the result is not
+            // a simple path.
+            if restriction.contains(pred) && *pred != pivot {
                 one_step.insert((*pred, *sign), (pivot, Positive));
             }
         }
diff --git a/src/_impl_regulatory_graph/signed_directed_graph/_feedback_vertex_set.rs b/src/_impl_regulatory_graph/signed_directed_graph/_feedback_vertex_set.rs
index 9575bc7..d5d92d3 100644
--- a/src/_impl_regulatory_graph/signed_directed_graph/_feedback_vertex_set.rs
+++ b/src/_impl_regulatory_graph/signed_directed_graph/_feedback_vertex_set.rs
@@ -224,7 +224,7 @@ impl SdGraph {
 mod tests {
     use crate::_impl_regulatory_graph::signed_directed_graph::SdGraph;
     use crate::_impl_regulatory_graph::signed_directed_graph::Sign::{Negative, Positive};
-    use crate::{BooleanNetwork, RegulatoryGraph, VariableId};
+    use crate::{BooleanNetwork, RegulatoryGraph, Sign, VariableId};
     use std::collections::HashSet;
 
     #[test]
@@ -303,4 +303,30 @@ mod tests {
         let expected = HashSet::from_iter([VariableId::from_index(2)]);
         assert_eq!(expected, bn.as_graph().feedback_vertex_set());
     }
+
+    #[test]
+    fn test_feedback_vertex_set_3() {
+        let bn = BooleanNetwork::try_from(
+            r#"
+        a3 -| a3
+        a5 -| a3
+        a6 -| a4
+        a4 -| a5
+        a4 -| a6
+        $a3: !a3 & !a5
+        $a4: !a6
+        $a5: !a4
+        $a6: !a4
+        "#,
+        )
+        .unwrap();
+
+        let p_fvs = bn.as_graph().parity_feedback_vertex_set(Sign::Positive);
+
+        // The graph only has one positive cycle on a4/a6.
+        let a4 = VariableId::from_index(1);
+        let a6 = VariableId::from_index(3);
+        assert!(p_fvs.contains(&a4) || p_fvs.contains(&a6));
+        assert_eq!(p_fvs.len(), 1);
+    }
 }