Merge pull request #261 from senhalil/feat/simplify_multi_pickup_or_m…

…ulti_delivery_shipments Simplify multi-pickup or multi-delivery shipments
Mapotempo · Aug 10, 2021 · cf716b0 · cf716b0
2 parents 721e32b + 0755733
commit cf716b0
Show file tree

Hide file tree

Showing 14 changed files with 417 additions and 48 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 - Implementation of `vehicle_trips` relation: the routes can be successive or with a minimum duration `lapse` in between [#123](https://github.com/Mapotempo/optimizer-api/pull/123)
 - CSV headers adapts to the language provided through HTTP_ACCEPT_LANGUAGE header to facilitate import in Mapotempo-Web [#196](https://github.com/Mapotempo/optimizer-api/pull/196)
 - Return route day/date and visits' index in result [#196](https://github.com/Mapotempo/optimizer-api/pull/196)
+- Treat complex shipments (multi-pickup-single-delivery and single-pickup-multi-delivery) as multiple simple shipments internally to increase performance [#261](https://github.com/Mapotempo/optimizer-api/pull/261)
 
 ### Changed
 

diff --git a/lib/helper.rb b/lib/helper.rb
@@ -63,6 +63,82 @@ def self.euclidean_distance(loc_a, loc_b)
     111321 * Math.sqrt(delta_lat**2 + delta_lon**2) # 111321 is the length of a degree (of lon and lat) in meters
   end
 
+  def self.deep_copy(original, override: {}, shallow_copy: [])
+    # TODO: after testing move the logic to base.rb as a clone function (initialize_copy)
+
+    # To override keys (and sub-keys) with values instead of using the originals
+    # (i.e., original[:key1], original[:other][:key2] etc) do
+    #                 override: { key1: value1, key2: value2 }
+    # To prevent deep copy and use the original objects and sub-objects
+    # (i.e., original[:key1], original[other][:key2]) do
+    #                 shallow_copy: [:key1, :key2 ]
+
+    # Assigning nil to a key in the override hash, skips the key and key_id of the objects
+
+    # WARNING: custom fields will be missing from the objects! because if a key/method doesn't exist
+    # in the Models::Class definition, it cannot be duplicated
+
+    case original
+    when Array # has_many
+      original.collect{ |val| deep_copy(val, override: override, shallow_copy: shallow_copy) }
+    when Models::Base # belongs_to
+      raise 'Keys cannot be both overridden and shallow copied' if (override.keys & shallow_copy).any?
+
+      # if an option doesn't exist for the current object level, pass it to lower levels
+      unused_override = override.select{ |key, _value|
+        [
+          key, "#{key}_id", "#{key[0..-2]}_ids", "#{key[0..-4]}y_ids", key[0..-4], "#{key[0..-6]}ies", "#{key[0..-5]}s"
+        ].none?{ |k| original.class.method_defined?(k.to_sym) }
+      }
+
+      unused_shallow_copy = shallow_copy.select{ |key|
+        [
+          key, "#{key}_id", "#{key[0..-2]}_ids", "#{key[0..-4]}y_ids", key[0..-4], "#{key[0..-6]}ies", "#{key[0..-5]}s"
+        ].none?{ |k| original.class.method_defined?(k.to_sym) }
+      }
+
+      # if a non-"id" version of the key exists, then prefer the hash of the non-id method (i.e., skip x_id(s))
+      # so that the objects are generated from scratch instead of re-used.
+      # Unless the key or key_id is marked as shallow_copy (then use the object) or the key or key_id is given in override.
+      keys = original.attributes.keys.flat_map{ |key|
+        [
+          key[0..-4].to_sym, "#{key[0..-6]}ies".to_sym, "#{key[0..-5]}s".to_sym, key
+        ].find{ |k| original.class.method_defined?(k) }
+      }.uniq - [:id] # cannot duplicate the object with the same id
+
+      # To reuse the same sub-members, the key needs to be given in the shallow_copy
+      # (which forces the duplication to use the original key object)
+      keys.map!{ |key|
+        if override.key?(key)
+          next unless override[key].nil?
+
+          [
+            "#{key}_id".to_sym, "#{key[0..-2]}_ids".to_sym, "#{key[0..-4]}y_ids".to_sym
+          ].find{ |k| original.class.method_defined?(k) && (!override.key?(k) || override[k]) }
+        elsif ["#{key}_id", "#{key[0..-2]}_ids", "#{key[0..-4]}y_ids"].any?{ |k| override[k.to_sym] }
+          next
+        else
+          key
+        end
+      }.compact!
+
+      # if a key is supplied in the override manually as nil, this means removing the key
+      # pass unused_override and unused_shallow_copy to the lower levels only
+      keys |= override.keys.select{ |k| override[k] && original.class.method_defined?(k) }
+      keys |= shallow_copy.select{ |k| original.class.method_defined?(k) }
+
+      # prefer the option if supplied
+      original.class.create(keys.each_with_object({}) { |key, data|
+        data[key] = override[key] ||
+                    (shallow_copy.include?(key) ? original.send(key) : deep_copy(original.send(key),
+                                                                                 override: unused_override,
+                                                                                 shallow_copy: unused_shallow_copy))
+      })
+    else
+      original.dup
+    end
+  end
+
   def self.merge_results(results, merge_unassigned = true)
     results.flatten!
     results.compact!

diff --git a/models/concerns/validate_data.rb b/models/concerns/validate_data.rb
@@ -322,12 +322,47 @@ def check_relations(periodic_heuristic)
     check_relation_consistent_timewindows
     check_sticky_relation_consistency
 
-    incompatible_relation_types = @hash[:relations].collect{ |r| r[:type] }.uniq - %i[force_first never_first force_end same_vehicle]
-    return unless periodic_heuristic && incompatible_relation_types.any?
+    if periodic_heuristic
+      incompatible_relation_types = @hash[:relations].collect{ |r| r[:type] }.uniq - %i[force_first never_first force_end same_vehicle]
+      err_msg = "#{incompatible_relation_types} relations not available with specified first_solution_strategy"
+      raise OptimizerWrapper::UnsupportedProblemError.new(err_msg) if incompatible_relation_types.any?
+    end
 
-    raise OptimizerWrapper::UnsupportedProblemError.new(
-      "#{incompatible_relation_types} relations not available with specified first_solution_strategy"
-    )
+    check_relation_visits_number_lapse_consistency
+  end
+
+  def check_relation_visits_number_lapse_consistency
+    # Do the check from exclusion point of view so that new relations can be considered automatically
+    relations_not_needing_matching_visits_number_and_lapses = %i[
+      force_end
+      force_first
+      never_first
+      same_vehicle
+      vehicle_trips
+      vehicle_group_duration
+      vehicle_group_duration_on_weeks
+      vehicle_group_duration_on_months
+      vehicle_group_number
+    ].freeze
+
+    @hash[:relations]&.each{ |relation|
+      next if relations_not_needing_matching_visits_number_and_lapses.include?(relation[:type].to_sym)
+
+      services_in_relation = relation[:linked_ids]&.collect{ |s_id| @hash[:services].find{ |s| s[:id] == s_id } } || []
+      if services_in_relation.uniq{ |s| s[:visits_number] || 1 }.size > 1
+        raise OptimizerWrapper::UnsupportedProblemError.new(
+          'Services in relations should have the same visits_number. '\
+          'Following services in relation but they have different visits_number values: ',
+          [relation[:linked_ids]]
+        )
+      elsif services_in_relation.uniq{ |s| [s[:minimum_lapse] || 1, s[:maximum_lapse].to_i] }.size > 1
+        raise OptimizerWrapper::UnsupportedProblemError.new(
+          'Services in relations should have the same minimum_lapse and maximum_lapse. '\
+          'Following services in relation but they have different lapse values: ',
+          [relation[:linked_ids]]
+        )
+      end
+    }
   end
 
   def check_sticky_relation_consistency

diff --git a/models/relation.rb b/models/relation.rb
@@ -33,6 +33,19 @@ class Relation < Base
     # validates_inclusion_of :type, :in => %i(same_vehicle same_route sequence order minimum_day_lapse maximum_day_lapse shipment meetup maximum_duration_lapse vehicle_group_duration vehicle_group_duration_on_weeks vehicle_group_duration_on_months vehicle_trips)
 
     def self.create(hash)
+      # TODO: remove it after the linked_ids is replaced with linked_service_ids in the api definition
+      exclusive = [:linked_service_ids, :linked_ids, :linked_services].freeze
+      raise "#{exclusive} fields are mutually exclusive" if hash.keys.count{ |k| exclusive.include? k } > 1
+
+      # TODO: remove it after the linked_ids is replaced with linked_service_ids in the api definition
+      if hash.key?(:linked_ids)
+        hash[:linked_service_ids] = hash[:linked_ids]
+      elsif hash.key?(:linked_service_ids)
+        hash[:linked_ids] = hash[:linked_service_ids]
+      elsif hash.key?(:linked_services)
+        hash[:linked_ids] = hash[:linked_services].map(&:id)
+      end
+
       hash[:type] = hash[:type]&.to_sym if hash.key?(:type)
       super(hash)
     end

diff --git a/models/vrp.rb b/models/vrp.rb
@@ -343,7 +343,6 @@ def self.filter(hash)
       self.remove_unnecessary_units(hash)
       self.remove_unnecessary_relations(hash)
       self.generate_schedule_indices_from_date(hash)
-      self.generate_linked_service_ids_for_relations(hash)
     end
 
     def self.remove_unnecessary_units(hash)
@@ -540,14 +539,6 @@ def self.generate_schedule_indices_from_date(hash)
       hash
     end
 
-    def self.generate_linked_service_ids_for_relations(hash)
-      hash[:relations]&.each{ |relation|
-        next unless relation[:linked_ids]&.any?
-
-        relation[:linked_service_ids] = relation[:linked_ids].select{ |id| hash[:services]&.any?{ |s| s[:id] == id } }
-      }
-    end
-
     def configuration=(configuration)
       self.config = configuration
       self.preprocessing = configuration[:preprocessing] if configuration[:preprocessing]

diff --git a/test/api/v01/with_solver_test.rb b/test/api/v01/with_solver_test.rb
@@ -42,7 +42,7 @@ def test_using_two_solver
     asynchronously start_worker: true do
       problem = VRP.lat_lon
       problem[:vehicles].first[:end_point_id] = nil
-      problem[:vehicles] << Marshal.load(Marshal.dump(problem[:vehicles].first))
+      problem[:vehicles] << Oj.load(Oj.dump(problem[:vehicles].first))
       problem[:vehicles].last[:id] = 'vehicle_1'
       problem[:vehicles].last[:start_point_id] = nil
 

diff --git a/test/fixtures/vrp_multipickup_singledelivery_shipments.dump b/test/fixtures/vrp_multipickup_singledelivery_shipments.dump
diff --git a/test/fixtures/vrp_multipickup_singledelivery_shipments.json b/test/fixtures/vrp_multipickup_singledelivery_shipments.json
diff --git a/test/models/vrp_test.rb b/test/models/vrp_test.rb
@@ -147,7 +147,7 @@ def test_deduce_consistent_relations
           lapse: 3
         }]
 
-        generated_vrp = TestHelper.create(Oj.load(Oj.dump(vrp)))
+        generated_vrp = TestHelper.create(vrp)
         assert_equal 3, generated_vrp.relations.size
         assert_equal 2, generated_vrp.relations.first.linked_ids.size
         assert_includes generated_vrp.relations.first.linked_ids, 'shipment_0_delivery'
@@ -163,7 +163,7 @@ def test_deduce_consistent_relations
         linked_ids: ['service', 'shipment_1'],
         lapse: 3
       }]
-      generated_vrp = TestHelper.create(Oj.load(Oj.dump(vrp)))
+      generated_vrp = TestHelper.create(vrp)
       assert_includes generated_vrp.relations.first.linked_ids, 'service'
       assert_includes generated_vrp.relations.first.linked_ids, 'shipment_1_pickup'
 
@@ -172,7 +172,7 @@ def test_deduce_consistent_relations
         linked_ids: ['service', 'shipment_0', 'shipment_1'],
         lapse: 3
       }]
-      generated_vrp = TestHelper.create(Oj.load(Oj.dump(vrp)))
+      generated_vrp = TestHelper.create(vrp)
       assert_includes generated_vrp.relations.first.linked_ids, 'service'
       assert_includes generated_vrp.relations.first.linked_ids, 'shipment_0_pickup'
       assert_includes generated_vrp.relations.first.linked_ids, 'shipment_1_pickup'
@@ -181,14 +181,14 @@ def test_deduce_consistent_relations
         vrp[:relations].first[:type] = relation_type
         vrp[:relations].first[:linked_ids] = ['service', 'shipment_1']
         assert_raises OptimizerWrapper::DiscordantProblemError do
-          TestHelper.create(Oj.load(Oj.dump(vrp)))
+          TestHelper.create(vrp)
         end
       }
 
       vrp[:relations].first[:linked_ids] = ['service']
       %i[sequence order].each{ |relation_type|
         vrp[:relations].first[:type] = relation_type
-        TestHelper.create(Oj.load(Oj.dump(vrp))) # check no error provided
+        TestHelper.create(vrp) # check no error provided
       }
     end
 

diff --git a/test/test_helper.rb b/test/test_helper.rb
@@ -143,7 +143,7 @@ def self.coerce(vrp)
   end
 
   def self.create(problem)
-    Models::Vrp.create(coerce(problem))
+    Models::Vrp.create(coerce(Oj.load(Oj.dump(problem))))
   end
 
   def self.matrices_required(vrps, filename)
@@ -163,8 +163,13 @@ def self.matrices_required(vrps, filename)
       else
         OptimizerWrapper.router.stub(:matrix, lambda { |url, mode, dimensions, row, column, options|
           if ENV['TEST_DUMP_VRP'] == 'true'
+            warn 'Overwriting the existing matrix dump' if dumped_data
+
+            WebMock.enable_net_connect!
             matrices =
               OptimizerWrapper.router.send(:__minitest_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 }
             matrices

diff --git a/test/wrapper_test.rb b/test/wrapper_test.rb
@@ -3224,6 +3224,40 @@ def test_cannot_simplify_service_setup_duration
     }
   end
 
+  def test_simplify_complex_multi_pickup_or_delivery_shipments
+    # check service count after simplification
+    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
+    simplifier_called = false
+    result_wo_simplify = Wrappers::Wrapper.stub_any_instance(:simplify_complex_multi_pickup_or_delivery_shipments,
+                                                             proc{
+                                                               simplifier_called = true
+                                                               nil
+                                                             }) do
+      vrp = TestHelper.load_vrp(self, fixture_file: 'vrp_multipickup_singledelivery_shipments')
+      OptimizerWrapper.wrapper_vrp('ortools', { services: { vrp: [:ortools] }}, vrp, nil)
+    end
+    assert simplifier_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_reject_when_unfeasible_timewindows
     vrp = VRP.toy
     vrp[:services].first[:activity][:timewindows] = [{ start: 0, end: 10 }, { start: 20, end: 15 }]

diff --git a/test/wrappers/ortools_test.rb b/test/wrappers/ortools_test.rb
@@ -3863,7 +3863,7 @@ def test_pud_initial_routes
 
     OptimizerWrapper.config[:services][:ortools].stub(
       :run_ortools,
-      lambda { |problem, _, _, _, _, _|
+      lambda { |problem, _, _|
         # check no service has been filtered :
         assert_equal expecting, (problem.routes.collect{ |r| r.service_ids.size })