From 5f663bdfb9f5e38616bee3e034a907a879f8cb25 Mon Sep 17 00:00:00 2001
From: Laurent Perron <lperron@google.com>
Date: Fri, 8 Nov 2024 13:52:22 -0800
Subject: [PATCH] fix set cover bug

---
 ortools/algorithms/BUILD.bazel             |  1 +
 ortools/algorithms/set_cover_heuristics.cc | 15 +++++++++++----
 ortools/algorithms/set_cover_test.cc       | 22 ++++++++++++++++++++--
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/ortools/algorithms/BUILD.bazel b/ortools/algorithms/BUILD.bazel
index df408f9927..4910c8799f 100644
--- a/ortools/algorithms/BUILD.bazel
+++ b/ortools/algorithms/BUILD.bazel
@@ -367,6 +367,7 @@ cc_test(
         ":set_cover_invariant",
         ":set_cover_mip",
         ":set_cover_model",
+        "//ortools/base:parse_text_proto",
         "//ortools/base:gmock_main",
         "@com_google_absl//absl/log",
         "@com_google_absl//absl/log:check",
diff --git a/ortools/algorithms/set_cover_heuristics.cc b/ortools/algorithms/set_cover_heuristics.cc
index 0f40c7ef8d..6e19eaaaa8 100644
--- a/ortools/algorithms/set_cover_heuristics.cc
+++ b/ortools/algorithms/set_cover_heuristics.cc
@@ -659,7 +659,8 @@ bool GuidedLocalSearch::NextSolution(absl::Span<const SubsetIndex> focus,
     }
   }
 
-  for (int iteration = 0; iteration < num_iterations; ++iteration) {
+  for (int iteration = 0;
+       !priority_heap_.IsEmpty() && iteration < num_iterations; ++iteration) {
     // Improve current solution respective to the current penalties.
     const SubsetIndex best_subset(priority_heap_.TopIndex());
     if (inv_->is_selected()[best_subset]) {
@@ -671,8 +672,10 @@ bool GuidedLocalSearch::NextSolution(absl::Span<const SubsetIndex> focus,
            best_subset.value()});
     }
     inv_->Flip(best_subset, CL::kRedundancy);  // Flip the best subset.
+    DCHECK(!utility_heap_.IsEmpty());
 
-    // Getting the subset with highest utility.
+    // Getting the subset with highest utility. utility_heap_ is not empty,
+    // because we just inserted a pair.
     const SubsetIndex penalized_subset(utility_heap_.TopIndex());
     utility_heap_.Pop();
     ++penalties_[penalized_subset];
@@ -680,6 +683,7 @@ bool GuidedLocalSearch::NextSolution(absl::Span<const SubsetIndex> focus,
         {static_cast<float>(inv_->model()->subset_costs()[penalized_subset] /
                             (1 + penalties_[penalized_subset])),
          penalized_subset.value()});
+    DCHECK(!utility_heap_.IsEmpty());
 
     // Get removable subsets (Add them to the heap).
     for (const SubsetIndex subset : inv_->newly_removable_subsets()) {
@@ -687,6 +691,7 @@ bool GuidedLocalSearch::NextSolution(absl::Span<const SubsetIndex> focus,
                                     inv_->model()->subset_costs()[subset]);
       priority_heap_.Insert({delta_selected, subset.value()});
     }
+    DCHECK(!priority_heap_.IsEmpty());
 
     for (const SubsetIndex subset : {penalized_subset, best_subset}) {
       const float delta = ComputeDelta(subset);
@@ -694,9 +699,11 @@ bool GuidedLocalSearch::NextSolution(absl::Span<const SubsetIndex> focus,
         priority_heap_.Insert({delta, subset.value()});
       }
     }
+    DCHECK(!priority_heap_.IsEmpty());
 
-    // Get new non removable subsets.
-    // (Delete them from the heap)
+    // Get new non removable subsets and remove them from the heap.
+    // This is when the priority_heap_ can become empty and end the outer loop
+    // early.
     for (const SubsetIndex subset : inv_->newly_non_removable_subsets()) {
       priority_heap_.Remove(subset.value());
     }
diff --git a/ortools/algorithms/set_cover_test.cc b/ortools/algorithms/set_cover_test.cc
index 9fab7b3a32..2124fd9a87 100644
--- a/ortools/algorithms/set_cover_test.cc
+++ b/ortools/algorithms/set_cover_test.cc
@@ -25,12 +25,30 @@
 #include "ortools/algorithms/set_cover_model.h"
 #include "ortools/base/gmock.h"
 #include "ortools/base/logging.h"
+#include "ortools/base/parse_text_proto.h"
 
 namespace operations_research {
 namespace {
-
+using google::protobuf::contrib::parse_proto::ParseTextProtoOrDie;
 using CL = SetCoverInvariant::ConsistencyLevel;
 
+TEST(SetCoverTest, GuidedLocalSearchVerySmall) {
+  SetCoverProto proto = ParseTextProtoOrDie(R"pb(
+    subset { cost: 1 element: 1 element: 2 }
+    subset { cost: 1 element: 0 })pb");
+
+  SetCoverModel model;
+  model.ImportModelFromProto(proto);
+  CHECK(model.ComputeFeasibility());
+  SetCoverInvariant inv(&model);
+  GreedySolutionGenerator greedy_search(&inv);
+  CHECK(greedy_search.NextSolution());
+  CHECK(inv.CheckConsistency(CL::kFreeAndUncovered));
+  GuidedLocalSearch search(&inv);
+  CHECK(search.NextSolution(100));
+  CHECK(inv.CheckConsistency(CL::kRedundancy));
+}
+
 SetCoverModel CreateKnightsCoverModel(int num_rows, int num_cols) {
   SetCoverModel model;
   constexpr int knight_row_move[] = {2, 1, -1, -2, -2, -1, 1, 2};
@@ -335,7 +353,7 @@ TEST(SetCoverTest, KnightsCoverElementDegreeRandomClear) {
   SetCoverInvariant inv(&model);
   Cost best_cost = std::numeric_limits<Cost>::max();
   SubsetBoolVector best_choices;
-  for (int i = 0; i < 1000; ++i) {
+  for (int i = 0; i < 10000; ++i) {
     LazyElementDegreeSolutionGenerator degree(&inv);
     CHECK(degree.NextSolution());
     CHECK(inv.CheckConsistency(CL::kCostAndCoverage));