Merge pull request #296 from senhalil/feat/fix_#688_cluserting_empty_…

…sub_vrp

Fixes #688 don't cluster empty vrp & ensure complete result

lib/interpreters/split_clustering.rb

@@ -49,15 +49,15 @@ class SplitClustering
     def self.split_clusters(service_vrp, job = nil, &block)
       vrp = service_vrp[:vrp]
 
-      if vrp.preprocessing_partitions && !vrp.preprocessing_partitions.empty?
+      if vrp.preprocessing_partitions&.any?
         splited_service_vrps = generate_split_vrps(service_vrp, job, block)
 
         OutputHelper::Clustering.generate_files(splited_service_vrps, vrp.preprocessing_partitions.size == 2, job) if OptimizerWrapper.config[:debug][:output_clusters] && service_vrp.size < splited_service_vrps.size
 
         split_results = splited_service_vrps.each_with_index.map{ |split_service_vrp, i|
           cluster_ref = i + 1
           result = OptimizerWrapper.define_process(split_service_vrp, job) { |wrapper, avancement, total, message, cost, time, solution|
-            msg = "process #{cluster_ref}/#{splited_service_vrps.size} - #{message}" unless message.nil?
+            msg = "split partition process #{cluster_ref}/#{splited_service_vrps.size} - #{message}" unless message.nil?
             block&.call(wrapper, avancement, total, msg, cost, time, solution)
           }
 
@@ -80,6 +80,11 @@ def self.split_clusters(service_vrp, job = nil, &block)
           }
         }
 
+        # merge expanded vehicles, services and relations
+        vrp.vehicles = splited_service_vrps.flat_map{ |sv| sv[:vrp].vehicles }
+        vrp.services = splited_service_vrps.flat_map{ |sv| sv[:vrp].services }
+        vrp.relations = splited_service_vrps.flat_map{ |sv| sv[:vrp].relations }
+
         return Helper.merge_results(split_results)
       elsif split_solve_candidate?(service_vrp)
         return split_solve(service_vrp, &block)
@@ -759,7 +764,7 @@ def self.split_balanced_kmeans(service_vrp, nb_clusters, options = {}, &block)
       # Split using balanced kmeans
       if vrp.services.all?{ |service| service.activity&.point&.location }
         result_items =
-          if nb_clusters > 1
+          if nb_clusters > 1 && vrp.services.any?
             cumulated_metrics = Hash.new(0)
 
             if options[:entity] == :work_day || !vrp.matrices.empty?
@@ -794,7 +799,14 @@ def self.split_balanced_kmeans(service_vrp, nb_clusters, options = {}, &block)
                 grouped_objects[i[2]]
               }
             }
+          elsif vrp.services.empty?
+            # this is normal if there is split independent vrp (by skills or sticky)
+            # we pass here so that result is collected correctly for all expanded vehicles
+            log 'Split is not possible if there is nothing to split', level: :debug
+
+            Array.new(nb_clusters){ [] } # mimics grouped_objects
           else
+            # nb_clusters == 1
             # this is normal if there is one vehicle (or one workday) and partitioning by vehicle (or workday) is active
             log 'Split is not possible if the cluster size is 1', level: :warn
 
@@ -805,8 +817,6 @@ def self.split_balanced_kmeans(service_vrp, nb_clusters, options = {}, &block)
 
         if options[:build_sub_vrps]
           result_items.collect.with_index{ |result_item, result_index|
-            next if result_item.empty?
-
             vehicles_indices = [result_index] if options[:entity] == :work_day || options[:entity] == :vehicle
             # TODO: build_partial_service_vrp can work directly with the list of services instead of ids.
             build_partial_service_vrp(service_vrp, result_item.collect(&:id), vehicles_indices, options[:entity])

optimizer_wrapper.rb

@@ -129,9 +129,14 @@ def self.define_main_process(services_vrps, job = nil, &block)
             msg = "#{"repetition #{repetition_index + 1}/#{service_vrp_repeats.size} - " if service_vrp_repeats.size > 1}#{message}" unless message.nil?
             callback_join&.call(wrapper, avancement, total, msg, cost, time, solution)
           }
-          Models.delete_all if service_vrp_repeats.size > 1 # needed to prevent duplicate ids because expand_repeat uses Marshal.load/dump
+          Models.delete_all # needed to prevent duplicate ids because expand_repeat uses Marshal.load/dump
+
+          break if repeated_results.last[:unassigned].empty? # No need to repeat more, cannot do better than this
         }
 
+        # NOTE: the only criteria is number of unassigneds at the moment so if there is ever a solution with zero
+        # unassigned, the loop is cut early. That is, if the criteria below is evolved, the above `break if` condition
+        # should be modified in a similar fashion)
         (result, position) = repeated_results.each.with_index(1).min_by { |result, _| result[:unassigned].size } # find the best result and its index
         log "#{job}_repetition - #{repeated_results.collect{ |r| r[:unassigned].size }} : chose to keep the #{position.ordinalize} solution"
         result
@@ -444,7 +449,7 @@ def self.join_independent_vrps(services_vrps, callback)
     results = services_vrps.each_with_index.map{ |service_vrp, i|
       block = if services_vrps.size > 1 && !callback.nil?
                 proc { |wrapper, avancement, total, message, cost = nil, time = nil, solution = nil|
-                  msg = "process #{i + 1}/#{services_vrps.size} - #{message}" unless message.nil?
+                  msg = "split independent process #{i + 1}/#{services_vrps.size} - #{message}" unless message.nil?
                   callback&.call(wrapper, avancement, total, msg, cost, time, solution)
                 }
               else

test/lib/heuristics/periodic_test.rb

@@ -269,7 +269,7 @@ def test_callage_freq
 
     def test_same_point_day_relaxation
       vrp = TestHelper.load_vrp(self)
-      result = OptimizerWrapper.wrapper_vrp('demo', { services: { vrp: [:demo] }}, vrp, nil)
+      result = OptimizerWrapper.wrapper_vrp('demo', { services: { vrp: [:demo] }}, Marshal.load(Marshal.dump(vrp)), nil)
 
       assert_equal vrp.visits, result[:routes].sum{ |route| route[:activities].count{ |stop| stop[:service_id] } } + result[:unassigned].size,
                    "Found #{result[:routes].sum{ |route| route[:activities].count{ |stop| stop[:service_id] } } + result[:unassigned].size} instead of #{vrp.visits} expected"

test/lib/interpreters/split_clustering_test.rb

@@ -199,7 +199,7 @@ def test_length_centroid
 
       services_vrps = Interpreters::SplitClustering.generate_split_vrps({ vrp: vrp, service: :demo }, nil, nil)
       assert services_vrps
-      assert_equal 2, services_vrps.size
+      assert_equal 35, services_vrps.size
     end
 
     def test_work_day_without_vehicle_entity_small

test/real_cases_periodic_solver_test.rb

@@ -28,7 +28,6 @@ def test_periodic_and_ortools
         unassigned = result[:unassigned].size
 
         vrp.resolution_solver = true
-        Models.delete_all # needed to prevent duplicate ids while calling wrapper_vrp second time
         result = OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, vrp, nil)
         assert unassigned >= result[:unassigned].size, "Increased number of unassigned with ORtools : had #{unassigned}, has #{result[:unassigned].size} now"
       }
@@ -38,7 +37,7 @@ def test_two_phases_clustering_sched_with_freq_and_same_point_day_5veh_with_solv
       vrp = TestHelper.load_vrp(self, fixture_file: 'two_phases_clustering_sched_with_freq_and_same_point_day_5veh')
       vrp.resolution_solver = true
       vrp.preprocessing_partitions.each{ |p| p.restarts = 1 }
-      result = OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, vrp, nil)
+      result = OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, Marshal.load(Marshal.dump(vrp)), nil)
 
       vrp[:services].group_by{ |s| s[:activity][:point][:id] }.each{ |point_id, services_set|
         expected_number_of_days = services_set.collect{ |service| service[:visits_number] }.max

test/real_cases_periodic_test.rb

@@ -123,7 +123,7 @@ def test_vrp_allow_partial_assigment_false
     def test_minimum_stop_in_route
       vrp = TestHelper.load_vrps(self, fixture_file: 'performance_13vl')[25]
       vrp.resolution_allow_partial_assignment = true
-      result = OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, vrp, nil)
+      result = OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, Marshal.load(Marshal.dump(vrp)), nil)
       assert result[:routes].any?{ |r| r[:activities].size - 2 < 5 },
              'We expect at least one route with less than 5 services, this test is useless otherwise'
       should_remain_assigned = result[:routes].sum{ |r| r[:activities].size - 2 >= 5 ? r[:activities].size - 2 : 0 }
@@ -135,7 +135,7 @@ def test_minimum_stop_in_route
       assert result[:routes].all?{ |r| (r[:activities].size - 2).zero? || r[:activities].size - 2 >= 5 },
              'Expecting no route with less than 5 stops unless it is an empty route'
       assert_operator should_remain_assigned, :<=, (result[:routes].sum{ |r| r[:activities].size - 2 })
-      assert_equal 19, result[:unassigned].size
+      assert_equal 25, result[:unassigned].size
 
       all_ids = (result[:routes].flat_map{ |route| route[:activities].collect{ |stop| stop[:service_id] } }.compact +
                 result[:unassigned].collect{ |un| un[:service_id] }).uniq

test/real_cases_test.rb

@@ -148,7 +148,7 @@ def test_vrp_ten_routes_with_rest
       check_vrp_services_size = vrp.services.size
 
       # or-tools performance
-      result = OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, vrp, nil)
+      result = OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, Marshal.load(Marshal.dump(vrp)), nil)
       assert result
       # Check activities
       assert_equal check_vrp_services_size, (result[:routes].sum{ |r| r[:activities].count{ |a| a[:service_id] } })

test/wrapper_test.rb

@@ -2815,9 +2815,9 @@ def test_detecting_unfeasible_services_can_not_take_too_long
     # corrected/increased. On local, the total_times are almost half the limits.
     # The goal of this test is to prevent adding an involuntary exponential logic and the limits can increase linearly
     # if more verifications are added but the time should not jump orders of magnitude.
-    assert_operator total_check_distances_time, :<=, 3.0, 'check_distances function took longer than expected'
-    assert_operator total_add_unassigned_time, :<=, 6.0, 'add_unassigned function took longer than expected'
-    assert_operator total_detect_unfeasible_services_time, :<=, 8.0, 'detect_unfeasible_services function took too long'
+    assert_operator total_check_distances_time, :<=, 3.3, 'check_distances function took longer than expected'
+    assert_operator total_add_unassigned_time, :<=, 6.6, 'add_unassigned function took longer than expected'
+    assert_operator total_detect_unfeasible_services_time, :<=, 8.8, 'detect_unfeasible_services function took too long'
   ensure
     OptimizerLogger.level = old_logger_level if old_logger_level
     OptimizerWrapper.config[:solve][:repetition] = old_config_solve_repetition if old_config_solve_repetition

test/wrappers/ortools_test.rb

@@ -4983,7 +4983,8 @@ def test_minimum_duration_lapse_shipments
     delivery1 = vrp.services.find{ |service| service.id == "#{ordered_pickup_ids[1]}_delivery" }
     pickup0 = vrp.services.find{ |service| service.id == "#{ordered_pickup_ids[0]}_pickup" }
 
-    # add consecutivity :
+    # add consecutivity:
+    vrp = TestHelper.load_vrp(self)
     vrp.relations << Models::Relation.create(
       type: :minimum_duration_lapse,
       linked_ids: [delivery1.id, pickup0.id],