Fix cpe update bug (#199)

* added comment describing bug

* added failing test

* added failing test: docstring fixed

* hacking about

* fixed test

* actual bug fixed

* full test

* update comment

* test still weird

* make test omit dropoffs

* removed complicated broken test

* trying to fix fast_forward_time

* new test

* test bug fixed

* mid-debug commit

* bug reason found, added failing test

* cleaned up test environment after determining the reason for the bug

* cleaned up test environment after determining the reason for the bug

* documented last_stop

.pre-commit-config.yaml

@@ -1,6 +1,6 @@
 repos:
 -   repo: https://github.com/python/black
-    rev: stable
+    rev: 22.3.0
     hooks:
     - id: black
       language_version: python3.9

ridepy/util/dispatchers/__init__.py

@@ -105,15 +105,10 @@ def TaxicabDispatcherDriveFirst(
     # TODO: When we have multi-passenger requests, this dispatcher needs to be changed and
     # include capacity constraints. Currently, taxi := single seat
     assert seat_capacity == 1
-    CPAT_pu = (
-        max(
-            stoplist[-1].estimated_arrival_time,
-            stoplist[-1].time_window_min
-            if stoplist[-1].time_window_min is not None
-            else 0,
-        )
-        + space.t(stoplist[-1].location, request.origin)
-    )
+    CPAT_pu = max(
+        stoplist[-1].estimated_arrival_time,
+        stoplist[-1].time_window_min if stoplist[-1].time_window_min is not None else 0,
+    ) + space.t(stoplist[-1].location, request.origin)
     EAST_pu = request.pickup_timewindow_min
     CPAT_do = max(EAST_pu, CPAT_pu) + space.t(request.origin, request.destination)
     LAST_pu = (
@@ -297,6 +292,12 @@ def BruteForceTotalTravelTimeMinimizingDispatcher(
 
     if min_cost < np.inf:
         best_pickup_idx, best_dropoff_idx = best_insertion
+        # if request.request_id == 2:
+        # print(f"Py DEBUG: best insertion @ {best_insertion}")
+        # print(stoplist[0].estimated_arrival_time)
+        # print(stoplist[0].location)
+        # print(request.creation_timestamp)
+        # print()
 
         logger.info(f"Best insertion: {best_insertion}")
         logger.info(f"Min cost: {min_cost}")

ridepy/util/dispatchers_cython/cdispatchers.h

@@ -150,6 +150,17 @@ InsertionResult<Loc> brute_force_total_traveltime_minimizing_dispatcher(
   if (min_cost < INFINITY) {
     int best_pickup_idx = best_insertion.first;
     int best_dropoff_idx = best_insertion.second;
+
+//    if (request->request_id == 2.) {
+//        {
+//        cout << "C++ DEBUG: best insertion @ (" << best_pickup_idx << ", " <<
+//        best_dropoff_idx << ")\n";
+//        cout << stoplist[0].estimated_arrival_time << endl;
+//        cout << stoplist[0].location << endl;
+//        cout << request->creation_timestamp << endl << endl;
+//
+//        }
+
     auto new_stoplist = insert_request_to_stoplist_drive_first(
         stoplist, request, best_pickup_idx, best_dropoff_idx, space);
     if (debug) {

ridepy/util/spaces.py

@@ -229,6 +229,7 @@ class Graph(TransportSpace):
     """
 
     n_dim = 1
+    loc_type = LocType.INT
 
     def _update_distance_cache(self):
         (

ridepy/util/spaces_cython/cspaces.h

@@ -19,6 +19,11 @@ using namespace std;
 namespace cstuff {
 typedef pair<double, double> R2loc;
 
+std::ostream& operator<<(std::ostream& stream, R2loc const& x)
+{
+    return stream << "(" << x.first << "," << x.second << ")" << endl;
+}
+
 template <typename Loc> class TransportSpace {
 public:
   double velocity;

ridepy/vehicle_state.py

@@ -2,7 +2,7 @@
 
 import numpy as np
 
-from typing import Optional, SupportsFloat, List
+from typing import Optional, SupportsFloat, List, Tuple
 
 from .data_structures import (
     Request,
@@ -90,6 +90,9 @@ def fast_forward_time(self, t: float) -> Tuple[List[StopEvent], List[Stop]]:
 
         event_cache = []
 
+        # Here, last_stop refers to the stop with the largest departure time value smaller or equal than t.
+        # This can either be the last stop in the stoplist that is serviced here, or it can be the
+        # (possibly outdated) CPE stop, of no other stop is serviced.
         last_stop = None
 
         # drop all non-future stops from the stoplist, except for the (outdated) CPE
@@ -140,22 +143,24 @@ def fast_forward_time(self, t: float) -> Tuple[List[StopEvent], List[Stop]]:
         self.stoplist[0].occupancy_after_servicing = last_stop.occupancy_after_servicing
 
         # set CPE location to current location as inferred from the time delta to the upcoming stop's CPAT
-        if len(self.stoplist) > 1:
-            if last_stop.estimated_arrival_time > t:
-                # still mid-jump from last interpolation, no need to interpolate
-                # again
-                pass
-            else:
-                self.stoplist[0].location, jump_time = self.space.interp_time(
+        # still mid-jump from last interpolation, no need to interpolate
+        # again
+        if self.stoplist[0].estimated_arrival_time <= t:
+            if len(self.stoplist) > 1:
+                loc, jump_time = self.space.interp_time(
                     u=last_stop.location,
                     v=self.stoplist[1].location,
                     time_to_dest=self.stoplist[1].estimated_arrival_time - t,
                 )
+                self.stoplist[0].location = loc
                 # set CPE time
                 self.stoplist[0].estimated_arrival_time = t + jump_time
-        else:
-            # stoplist is empty, only CPE is there. set CPE time to current time
-            self.stoplist[0].estimated_arrival_time = t
+            else:
+                # Stoplist is (now) empty, only CPE is there. Set CPE time to
+                # current time and move CPE to last_stop's location (which is
+                # identical to CPE, if we haven't served anything.
+                self.stoplist[0].location = last_stop.location
+                self.stoplist[0].estimated_arrival_time = t
 
         return event_cache
 

ridepy/vehicle_state_cython/cvehicle_state.h

@@ -64,14 +64,20 @@ template <typename Loc> class VehicleState {
     // TODO optionally validate the travel time velocity constraints
     vector<StopEventSpec> event_cache;
 
+    // Here, last_stop refers to the stop with the largest departure time value smaller or equal than t.
+    // This can either be the last stop in the stoplist that is serviced here, or it can be the
+    // (possibly outdated) CPE stop, of no other stop is serviced.
     Stop<Loc> last_stop;
+
     bool any_stop_serviced{false};
     // drop all non-future stops from the stoplist, except for the (outdated)
     // CPE
     for (int i = stoplist.size() - 1; i > 0; --i) {
       auto &stop = stoplist[i];
+      auto service_time = max(stop.estimated_arrival_time, stop.time_window_min);
+
       // service the stop at its estimated arrival time
-      if (stop.estimated_arrival_time <= t) {
+      if (service_time <= t) {
         // as we are iterating backwards, the first stop iterated over is the
         // last one serviced
         if (not any_stop_serviced) {
@@ -83,8 +89,7 @@ template <typename Loc> class VehicleState {
           any_stop_serviced = true;
         }
         event_cache.push_back(
-            {stop.action, stop.request->request_id, vehicle_id,
-             max(stop.estimated_arrival_time, stop.time_window_min)});
+            {stop.action, stop.request->request_id, vehicle_id, service_time});
         stoplist.erase(stoplist.begin() + i);
       }
     }
@@ -101,22 +106,24 @@ template <typename Loc> class VehicleState {
 
     // set CPE location to current location as inferred from the time delta to
     // the upcoming stop's CPAT
-    if (stoplist.size() > 1) {
-      if (last_stop.estimated_arrival_time > t) {
-        // still mid-jump from last interpolation, no need to interpolate
-        // again
-        1 == 1;
-      } else {
-        auto [loc, jump_time] =
-            space.interp_time(last_stop.location, stoplist[1].location,
-                              stoplist[1].estimated_arrival_time - t);
-        stoplist[0].location = loc;
-        // set CPE time
-        stoplist[0].estimated_arrival_time = t + jump_time;
-      }
-    } else {
-      // stoplist is empty, only CPE is there. set CPE time to current time
-      stoplist[0].estimated_arrival_time = t;
+    // still mid-jump from last interpolation, no need to interpolate
+    // again
+    if (stoplist[0].estimated_arrival_time <= t) {
+        if (stoplist.size() > 1) {
+            auto [loc, jump_time] =
+                space.interp_time(last_stop.location, stoplist[1].location,
+                                  stoplist[1].estimated_arrival_time - t);
+            stoplist[0].location = loc;
+            // set CPE time
+            stoplist[0].estimated_arrival_time = t + jump_time;
+        }
+        else {
+          // Stoplist is (now) empty, only CPE is there. Set CPE time to
+          // current time and move CPE to last_stop's location (which is
+          // identical to CPE, if we haven't served anything.
+          stoplist[0].location = last_stop.location;
+          stoplist[0].estimated_arrival_time = t;
+        }
     }
     return event_cache;
   }

test/benchmark_cython_bruteforce_dispatcher.py

@@ -56,7 +56,9 @@ def benchmark_insertion_into_long_stoplist(seed=0):
         vehicle_id=12,
         initial_stoplist=stoplist,
         space=space,
-        dispatcher=BruteForceTotalTravelTimeMinimizingDispatcher(loc_type=LocType.R2LOC),
+        dispatcher=BruteForceTotalTravelTimeMinimizingDispatcher(
+            loc_type=LocType.R2LOC
+        ),
         seat_capacity=1000,
     )
     request = TransportationRequest(
@@ -71,7 +73,7 @@ def benchmark_insertion_into_long_stoplist(seed=0):
     )
     tick = time()
     # TODO: instead of creating VehicleState, call cythonic dispatcher directly (same as the pythonic benchmark script)
-    #vs.handle_transportation_request_single_vehicle(request)
+    # vs.handle_transportation_request_single_vehicle(request)
     cythonic_solution = CyBruteForceTotalTravelTimeMinimizingDispatcher(LocType.R2LOC)(
         request, stoplist, space, seat_capacity=100
     )

test/test_bruteforce_dispatcher_cython.py

@@ -125,10 +125,12 @@ def test_equivalence_simulator_cython_and_python_bruteforce_dispatcher(seed=42):
     """
     for py_space, cy_space in (
         (pyspaces.Euclidean2D(), cyspaces.Euclidean2D()),
-        (
-            pyspaces.Graph.from_nx(make_nx_grid()),
-            cyspaces.Graph.from_nx(make_nx_grid()),
-        ),
+        # DO NOT test graph spaces for now, as we use different methods for computing the shortest path
+        # (floyd-warshall in Python and dijkstra in C++. Therefore differences in interpolation arise.)
+        # (
+        #     pyspaces.Graph.from_nx(make_nx_grid()),
+        #     cyspaces.Graph.from_nx(make_nx_grid()),
+        # ),
     ):
 
         n_reqs = 100

test/test_space.py

@@ -523,3 +523,18 @@ def test_caching_in_boost_graph_space():
 
     assert (np.array(results) == results[0]).all()
     assert compute_times[0] > compute_times[1]
+
+
+@pytest.mark.xfail()
+# DO NOT test this for now, as we use different methods for computing the shortest path
+# (floyd-warshall in Python and dijkstra in C++. Therefore differences in interpolation arise.)
+def test_python_cython_graph_interpolation_equivalence():
+
+    pyspace = Graph.from_nx(make_nx_grid())
+    cyspace = CyGraph.from_nx(make_nx_grid())
+
+    py_loc, py_jump_time = pyspace.interp_time(0, 4, 1.5)
+    cy_loc, cy_jump_time = cyspace.interp_time(0, 4, 1.5)
+
+    assert py_loc == cy_loc
+    assert py_jump_time == cy_jump_time