Merge pull request #307 from senhalil/feat/fix_#882_violated_multi_pi…

…ckup_delivery

Fix #882 violated multi pickup delivery

README.md

@@ -58,9 +58,9 @@ gem install bundler
 bundle install
 ```
 
-This project requires some solver and interface projects in order to be fully functionnal!
+This project requires some solver and interface projects in order to be fully functional!
 * [Vroom v1.8.0](https://github.com/VROOM-Project/vroom/releases/tag/v1.8.0)
-* [Optimizer-ortools v1.9.0](https://github.com/Mapotempo/optimizer-ortools) & [OR-Tools v7.8](https://github.com/google/or-tools/releases/tag/v7.8) (use the version corresponding to your system operator, not source code).
+* [Optimizer-ortools v1.10.0](https://github.com/Mapotempo/optimizer-ortools) & [OR-Tools v7.8](https://github.com/google/or-tools/releases/tag/v7.8) (use the version corresponding to your system operator, not source code).
 
 Note : when updating OR-Tools you should to recompile optimizer-ortools.
 

docker/Dockerfile

@@ -8,7 +8,7 @@ ARG VROOM_VERSION
 FROM vroomvrp/vroom-docker:${VROOM_VERSION:-v1.8.0} as vroom
 
 # Rake
-FROM ${REGISTRY:-registry.mapotempo.com/}mapotempo-${BRANCH:-ce}/optimizer-ortools:${OPTIMIZER_ORTOOLS_VERSION:-v1.9.0}
+FROM ${REGISTRY:-registry.mapotempo.com/}mapotempo-${BRANCH:-ce}/optimizer-ortools:${OPTIMIZER_ORTOOLS_VERSION:-v1.10.0}
 ARG BUNDLE_WITHOUT
 
 ENV LANG C.UTF-8

test/test_helper.rb

@@ -46,6 +46,7 @@
     DichotomiousTest#test_dichotomious_approach
     RealCasesTest#test_ortools_open_timewindows
     SplitClusteringTest#test_avoid_capacities_overlap
+    SplitClusteringTest#test_cluster_one_phase_vehicle
     SplitClusteringTest#test_instance_same_point_day
     SplitClusteringTest#test_no_doubles_3000
     WrapperTest#test_detecting_unfeasible_services_can_not_take_too_long
@@ -201,7 +202,7 @@ def self.matrices_required(vrps, filename)
 
             WebMock.enable_net_connect!
             matrices =
-              vrp.router.send(:__minitest_stub__matrix, url, mode, dimensions, row, column, options) # call original method
+              vrp.router.send(:__minitest_any_instance_stub__matrix, url, mode, dimensions, row, column, options) # call original method
             WebMock.disable_net_connect!
             write_in_dump <<
               { url: url, mode: mode, dimensions: dimensions, row: row, column: column, options: options, matrices: matrices }

test/wrapper_test.rb

@@ -3377,38 +3377,57 @@ def test_prioritize_first_available_trips_and_vehicles
     assert there_is_a_skipped_trip_when_simplification_is_off, assert_msg
   end
 
-  def test_simplify_complex_multi_pickup_or_delivery_shipments
-    # check service count after simplification
+  def test_protobuf_receives_correct_simplified_complex_shipments
     vrp = TestHelper.load_vrp(self, fixture_file: 'vrp_multipickup_singledelivery_shipments')
-    service_count = vrp.services.size
-    OptimizerWrapper.config[:services][:demo].simplify_complex_multi_pickup_or_delivery_shipments(vrp)
-    assert_equal service_count + 7, vrp.services.size, 'simplify should have created 7 new services'
 
-    # Solve WITHOUT simplification
-    function_called = false
-    result_wo_simplify = Wrappers::Wrapper.stub_any_instance(:simplify_complex_multi_pickup_or_delivery_shipments,
-                                                             proc{
-                                                               function_called = true
-                                                               nil
-                                                             }) do
-      vrp = TestHelper.load_vrp(self, fixture_file: 'vrp_multipickup_singledelivery_shipments')
-      OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, vrp, nil)
+    assert_raises OptimizerWrapper::JobKilledError do
+      OptimizerWrapper.config[:services][:ortools].stub(
+        :run_ortools,
+        proc{ |problem, _vrp, _thread_proc, _block|
+          # there are 7 multi-pickup-single-delivery P&Ds so the stats should be as follows:
+          err_msg = 'Simplified multi-pickup-single-delivery p&d relation count is not correct'
+          assert_equal 7, (problem.relations.count{ |r| r.type == 'sequence' }), err_msg
+          assert_equal 20, (problem.relations.count{ |r| r.type == 'shipment' }), err_msg
+          assert_equal 54, problem.relations.flat_map(&:linked_ids).size, err_msg
+          assert_equal 40, problem.relations.flat_map(&:linked_ids).uniq.size, err_msg
+
+          raise OptimizerWrapper::JobKilledError # Return "Job killed" to stop gracefully
+        }
+      ) do
+        OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, vrp, nil)
+      end
     end
-    assert function_called, 'simplify_complex_multi_pickup_or_delivery_shipments should have been called'
-    # if there are no unassigned; maybe or-tools improved its performance
-    # and this simplification might not be necessary anymore,
-    # or this instance is too small and timing needs to be fixed.
-    # In any case, testing with a bigger instance might be necessary if the following condition fails regularly.
-    refute_empty result_wo_simplify[:unassigned], 'There should have been some unassigned'
-
-    # Solve WITH simplification
-    vrp = TestHelper.load_vrp(self, fixture_file: 'vrp_multipickup_singledelivery_shipments')
-    result_w_simplify = OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, vrp, nil)
-    assert_operator result_w_simplify[:unassigned].size, :<, result_wo_simplify[:unassigned].size,
-                    'Simplification should improve performance'
-    # if there are unassigned services; this might be due to computer performance
-    # but normally even 0.5 seconds is enough, so it might be due to a change in or-tools or optimizer-ortools
-    assert_empty result_w_simplify[:unassigned], 'Simplification should plan all services'
+  end
+
+  def test_simplified_complex_shipments_respect_the_original_relations
+    vrp = VRP.basic
+
+    vrp[:relations] = [
+      { type: :shipment, linked_ids: %w[service_1 service_3] },
+      { type: :shipment, linked_ids: %w[service_2 service_3] },
+    ]
+
+    vrp[:vehicles] << vrp[:vehicles].first.merge({id: 'vehicle_2', cost_fixed: 1 })
+
+    # The matrix makes it so that if we ignore the complex shipment, serving s1 and s2 in two different vehicles is a
+    # lot cheaper (4) then serving them on one vehicle (102). So we verify if optim-api does the "right" thing even if
+    # it is inconvenient.
+    vrp[:matrices][0][:time] = [
+      [0, 1, 1, 1],
+      [1, 0, 100, 1], # pickup1 to pickup2 is hard
+      [1, 101, 0, 1], # pickup2 to pickup1 is hard
+      [1, 11, 10, 0]  # delivery to p1 and p2 are hard
+    ]
+    vrp = TestHelper.create(vrp)
+
+    result = OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, vrp, nil)
+
+    assert_empty result[:unassigned], 'There should be no unsigned services'
+    assert_equal 1, result[:routes].count{ |r| r[:activities].any?{ |a| a[:service_id] } }, 'All services must be assigned to one vehicle'
+
+    planned_order = result[:routes][0][:activities].map{ |a| a[:service_id] }.compact
+    feasible_orders = [%w[service_1 service_2 service_3], %w[service_2 service_1 service_3]]
+    assert_includes feasible_orders, planned_order, 'Complex shipment relation is violated'
   end
 
   def test_reject_when_unfeasible_timewindows

wrappers/ortools.rb

@@ -289,18 +289,16 @@ def solve(vrp, job, thread_proc = nil, &block)
       }
 
       vrp.relations.each{ |relation|
-        relation.split_regarding_lapses.flat_map{ |relation_portion|
-          portion_linked_ids, portion_vehicle_ids, portion_lapse = relation_portion
-
-          current_linked_ids = (portion_linked_ids & services.map(&:id)).uniq if portion_linked_ids
-          current_linked_vehicles = (portion_vehicle_ids & vehicles.map(&:id)).uniq if portion_vehicle_ids
+        relation.split_regarding_lapses.each{ |portion_linked_ids, portion_vehicle_ids, portion_lapse|
+          current_linked_ids = (portion_linked_ids.map!(&:to_s) & services.map(&:id)).uniq if portion_linked_ids
+          current_linked_vehicles = (portion_vehicle_ids.map!(&:to_s) & vehicles.map(&:id)).uniq if portion_vehicle_ids
           next if current_linked_ids.to_a.empty? && current_linked_vehicles.to_a.empty?
 
           # NOTE: we collect lapse because optimizer-ortools expects one lapse per relation for now
           relations << OrtoolsVrp::Relation.new(
             type: relation.type,
-            linked_ids: current_linked_ids&.map(&:to_s),
-            linked_vehicle_ids: current_linked_vehicles&.map(&:to_s),
+            linked_ids: current_linked_ids,
+            linked_vehicle_ids: current_linked_vehicles,
             lapse: portion_lapse
           )
         }

wrappers/wrapper.rb

@@ -1079,14 +1079,14 @@ def simplify_complex_multi_pickup_or_delivery_shipments(vrp, result = nil, optio
                                            shallow_copy: [:unit, :point])
             sequence_relation_ids << new_service.id
 
-            if index > 0
+            if index < shipment_relations.size - 1
               # all timing will be handled by the first service (of the sequence relation)
               # correct the TW.end for the succeeding services (so that they can start)
               activity = new_service.activity
               activity.timewindows.each{ |tw| tw.end += activity.duration if tw&.end }
               activity.additional_value = 0
+              activity.setup_duration += activity.duration
               activity.duration = 0
-              activity.setup_duration = 0
 
               # inserting the remaining parts of a multipart service should be of highest priority
               new_service.priority = 0 # 0 is the highest priority
@@ -1112,7 +1112,9 @@ def simplify_complex_multi_pickup_or_delivery_shipments(vrp, result = nil, optio
             expanded_services << new_service
           }
 
-          sequence_relations << Models::Relation.create(type: :sequence, linked_ids: sequence_relation_ids)
+          # For some reason, or-tools performance is better when the sequence relation is defined in the inverse order.
+          # Note that, activity.duration's are set to zero except the last duplicated service (so we model exactly same constraint). 
+          sequence_relations << Models::Relation.create(type: :sequence, linked_ids: sequence_relation_ids.reverse)
         }
 
         vrp[:simplified_complex_shipments] = {
@@ -1181,6 +1183,9 @@ def simplify_complex_multi_pickup_or_delivery_shipments(vrp, result = nil, optio
 
               next unless insert_location # expanded activity(ies) of service is found in this route
 
+              # stop.. something went wrong if duplicated services are planned on different vehicles
+              raise 'Simplification cannot patch the result if duplicated services are planned on different vehicles' unless deleted_exp_ser_count == planned_exp_ser_count
+
               merged_activity = first_exp_ser_activity.merge(last_exp_ser_activity) { |key, first, last|
                 if key == :service_id
                   service.id
@@ -1438,7 +1443,7 @@ def simplify_service_setup_duration_and_vehicle_setup_modifiers(vrp, result = ni
         # simplifies the constraint
         return nil if vrp.vehicles.group_by(&:matrix_id).any?{ |_m_id, v_group|
                         v_group.group_by{ |v| [v.coef_setup || 1, v.additional_setup.to_i] }.size > 1
-                      }
+                      } || vrp.services.any?{ |s| s.activity.nil? }
 
         vehicles_grouped_by_matrix_id = vrp.vehicles.group_by(&:matrix_id)
         vrp.services.group_by{ |s| s.activity.point }.each{ |point, service_group|
@@ -1469,7 +1474,7 @@ def simplify_service_setup_duration_and_vehicle_setup_modifiers(vrp, result = ni
           }
         }
 
-        return nil unless vrp.services.any?{ |s| s[:simplified_setup_duration] }
+        return nil unless vrp.services.any?{ |s| s.activity[:simplified_setup_duration] }
 
         simplification_active = true
 
@@ -1481,14 +1486,14 @@ def simplify_service_setup_duration_and_vehicle_setup_modifiers(vrp, result = ni
         }
       when :rewind
         # take it back in case in dicho and there will be re-optimization
-        return nil unless vrp.services.any?{ |s| s[:simplified_setup_duration] }
+        return nil unless vrp.services.any?{ |s| s.activity[:simplified_setup_duration] }
 
         simplification_active = true
 
         vehicles_grouped_by_matrix_id = vrp.vehicles.group_by(&:matrix_id)
 
         vrp.services.group_by{ |s| s.activity.point }.each{ |point, service_group|
-          setup_duration = service_group.first[:simplified_setup_duration].to_i
+          setup_duration = service_group.first.activity[:simplified_setup_duration].to_i
 
           next if setup_duration.zero?
 
@@ -1503,8 +1508,8 @@ def simplify_service_setup_duration_and_vehicle_setup_modifiers(vrp, result = ni
           }
 
           service_group.each{ |service|
-            service.setup_duration = service[:simplified_setup_duration]
-            service[:simplified_setup_duration] = nil
+            service.activity.setup_duration = service.activity[:simplified_setup_duration]
+            service.activity[:simplified_setup_duration] = nil
           }
         }
 
@@ -1518,12 +1523,12 @@ def simplify_service_setup_duration_and_vehicle_setup_modifiers(vrp, result = ni
         # patches the result
         # the travel_times need to be decreased and setup_duration need to be increased by
         # (coef_setup * setup_duration + additional_setup) if setup_duration > 0 and travel_time > 0
-        return nil unless vrp.services.any?{ |s| s[:simplified_setup_duration] }
+        return nil unless vrp.services.any?{ |s| s.activity[:simplified_setup_duration] }
 
         simplification_active = true
 
         vehicles_grouped_by_vehicle_id = vrp.vehicles.group_by(&:id)
-        services_grouped_by_point_id = vrp.services.group_by{ |s| s.activity.point }
+        services_grouped_by_point_id = vrp.services.group_by{ |s| s.activity.point_id }
 
         overall_total_travel_time_correction = 0
         result[:routes].each{ |route|
@@ -1533,9 +1538,9 @@ def simplify_service_setup_duration_and_vehicle_setup_modifiers(vrp, result = ni
 
           total_travel_time_correction = 0
           route[:activities].each{ |activity|
-            next if activity[:travel_time].to_i.zero?
+            next if activity[:service_id].nil? || activity[:travel_time].to_i.zero?
 
-            setup_duration = services_grouped_by_point_id[activity[:point_id]].first[:simplified_setup_duration].to_i
+            setup_duration = services_grouped_by_point_id[activity[:point_id]].first.activity[:simplified_setup_duration].to_i
 
             next if setup_duration.zero?
 
@@ -1552,7 +1557,7 @@ def simplify_service_setup_duration_and_vehicle_setup_modifiers(vrp, result = ni
         result[:total_travel_time] -= overall_total_travel_time_correction.round
 
         result[:unassigned].each{ |activity|
-          setup_duration = services_grouped_by_point_id[activity[:point_id]].first[:simplified_setup_duration].to_i
+          setup_duration = services_grouped_by_point_id[activity[:point_id]].first.activity[:simplified_setup_duration].to_i
 
           activity[:detail][:setup_duration] = setup_duration
         }