Remove the global Calculator object

What's left in the initialization of the Calculator class is lightweight and self contained. Instead of having a long live Calculator object, we now instanciate an object at the time of running the appropriate calculation. We call the initializeCalculationData in the constructor now. (We could merge them later). We also initialize the algorithmCalculationTime in the constructor, as we assume we will create the object and run the calculation right away. (And we made it private)
chairemobilite · Nov 24, 2023 · 5a0755f · 5a0755f
1 parent 4a61e26
commit 5a0755f
Show file tree

Hide file tree

Showing 9 changed files with 26 additions and 83 deletions.
diff --git a/connection_scan_algorithm/include/calculator.hpp b/connection_scan_algorithm/include/calculator.hpp
@@ -78,23 +78,16 @@ namespace TrRouting
 
     std::vector<int>        optimizeJourney(std::deque<JourneyStep> &journey);
 
-    // Public for testing, this function initializes the calculation vectors and should be called whenever nodes and schedules are updated
-    // TODO As part of issue https://github.com/chairemobilite/trRouting/issues/95, this will be removed
-    void initializeCalculationData();
-
-    CalculationTime algorithmCalculationTime;
-
-    // TODO Added Glob suffix to easily track which one was local and which was global
-    std::optional<std::reference_wrapper<const OdTrip>> odTripGlob;
-
   private:
+    void initializeCalculationData();
     bool resetAccessFootpaths(const CommonParameters &parameters, const Point& origin);
     bool resetEgressFootpaths(const CommonParameters &parameters, const Point& destination);
     void resetFilters(const CommonParameters &parameters);
     // Convert the optimization case ID returned by optimizeJourney to a string
     std::string optimizeCasesToString(const std::vector<int> optimizeCases);
     std::unique_ptr<SingleCalculationResult> calculateSingleReverse(RouteParameters &parameters);
 
+    CalculationTime algorithmCalculationTime;
     //TODO set it mutable so it can be changed/reset?
     const TransitData &transitData;
     //TODO Should it be const?
@@ -108,6 +101,9 @@ namespace TrRouting
     int              minEgressTravelTime;
     long long        calculationTime;
 
+    // TODO Added Glob suffix to easily track which one was local and which was global
+    std::optional<std::reference_wrapper<const OdTrip>> odTripGlob; //Used to tell the reset function that we are doing an OdTrip calculations
+
     std::vector<int> nodesTentativeTime; // arrival time at node, using the Node::id as index
     std::vector<int> nodesReverseTentativeTime; // departure time at node
     std::unordered_map<Node::uid_t, NodeTimeDistance> nodesAccess; // travel time/distance from origin to accessible nodes

diff --git a/connection_scan_algorithm/src/initializations.cpp b/connection_scan_algorithm/src/initializations.cpp
@@ -24,7 +24,6 @@ namespace TrRouting
 
   Calculator::Calculator(const TransitData &_transitData, GeoFilter &_geoFilter) :
     algorithmCalculationTime(CalculationTime()),
-    odTripGlob(std::nullopt),
     transitData(_transitData),
     geoFilter(_geoFilter),
     departureTimeSeconds(0),
@@ -33,11 +32,14 @@ namespace TrRouting
     maxEgressTravelTime(0),
     maxAccessTravelTime(0),
     minEgressTravelTime(0),
-    calculationTime(0)
+    calculationTime(0),
+    odTripGlob(std::nullopt)
   {
-
+    initializeCalculationData();
+    algorithmCalculationTime.start(); //Automatically start timer on construction
   }
 
+  //TODO Maybe merge into the constructor
   void Calculator::initializeCalculationData() {
     spdlog::info("preparing nodes tentative times, trips enter connections and journeys...");
 

diff --git a/connection_scan_algorithm/src/transit_routing_http_server.cpp b/connection_scan_algorithm/src/transit_routing_http_server.cpp
@@ -136,9 +136,6 @@ int main(int argc, char** argv) {
     spdlog::info("Using OSRM for access/egress node time/distance");
   }
 
-  Calculator calculator(transitData, *geoFilter);
-  //TODO, should this be in the constructor?
-  calculator.initializeCalculationData();
   spdlog::info("preparing server...");
 
   //HTTP-server using 1 thread
@@ -147,34 +144,8 @@ int main(int argc, char** argv) {
   HttpServer server;
   server.config.port = programOptions.port;
 
-  server.resource["^/saveCache[/]?$"]["POST"] = [&server, &calculator](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> request) {
-
-    std::string response {""};
-
-
-    try {
-      boost::property_tree::ptree pt;
-      boost::property_tree::read_json(request->content, pt);
-
-      //auto name = pt.get<string>("firstName") + " " + pt.get<string>("lastName");
-
-      //response = "{\"status\": \"error\", \"error\": \"missing or wrong cache name\"}";
-    }
-    catch(const std::exception &e) {
-      std::string error(e.what());
-      response = "{\"status\": \"error\", \"error\": \"" + error + "\"}";
-    }
-
-
-    response = "{\"status\": \"error\", \"error\": \"missing or wrong cache name\"}";
-
-    *serverResponse << "HTTP/1.1 200 OK\r\nAccess-Control-Allow-Origin: *\r\nContent-Type: application/json; charset=utf-8\r\nContent-Length: " << response.length() << "\r\n\r\n" << response;
-
-  };
-
-
   // updateCache:
-  server.resource["^/updateCache[/]?$"]["GET"]=[&server, &calculator, &transitData](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> request) {
+  server.resource["^/updateCache[/]?$"]["GET"]=[&server, &transitData](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> request) {
 
     std::string              response {""};
     std::vector<std::string> parametersWithValues;
@@ -293,7 +264,6 @@ int main(int argc, char** argv) {
     //TODO do this only if we had at least one correct name
     //Reinit some data after the update
     // TODO Just the schedules???
-    calculator.initializeCalculationData();
     if (cacheNames.size() > 0)
     {
       // Remove last ","
@@ -315,7 +285,7 @@ int main(int argc, char** argv) {
 
 
   // closeServer and exit app:
-  server.resource["^/exit[/]?\\?([0-9a-zA-Z&=_,:/.-]+)$"]["GET"]=[&server, &calculator](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> ) {
+  server.resource["^/exit[/]?\\?([0-9a-zA-Z&=_,:/.-]+)$"]["GET"]=[&server](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> ) {
 
     std::string response {""};
     *serverResponse << "HTTP/1.1 200 OK\r\nAccess-Control-Allow-Origin: *\r\nContent-Type: application/json; charset=utf-8\r\nContent-Length: " << response.length() << "\r\n\r\n" << response;
@@ -331,7 +301,7 @@ int main(int argc, char** argv) {
 
   // Routing request for a single origin destination
   // TODO Copy-pasted and adapted from /route/v1/transit. There's still a lot of common code. Application code should be extracted to common functions outside the web server
-  server.resource["^/v2/route[/]?$"]["GET"]=[&server, &calculator, &dataStatus, &transitData](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> request) {
+  server.resource["^/v2/route[/]?$"]["GET"]=[&server, &dataStatus, &transitData, &geoFilter](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> request) {
     // Have a global id to match the requests in the logs
     static int routeRequestId = 0;
     std::string response = getFastErrorResponse(dataStatus);
@@ -340,10 +310,7 @@ int main(int argc, char** argv) {
       *serverResponse << "HTTP/1.1 200 OK\r\nAccess-Control-Allow-Origin: *\r\nContent-Type: application/json; charset=utf-8\r\nContent-Length: " << response.length() << "\r\n\r\n" << response;
       return;
     }
-
-    // prepare timer:
-    // TODO Shouldn't have to do this, a query is not a benchmark
-    calculator.algorithmCalculationTime.start();
+    Calculator calculator(transitData, *geoFilter);
 
     // prepare parameters:
     std::vector<std::pair<std::string, std::string>> parametersWithValues;
@@ -404,7 +371,7 @@ int main(int argc, char** argv) {
 
   // Request a summary of lines data for a route
   // TODO Copy pasted from v2/route. There's a lot in common, it should be extracted to common class, just the response parser is different
-  server.resource["^/v2/summary[/]?$"]["GET"]=[&server, &calculator, &dataStatus, &transitData](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> request) {
+  server.resource["^/v2/summary[/]?$"]["GET"]=[&server, &dataStatus, &transitData, &geoFilter](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> request) {
     // Have a global id to match the requests in the logs
     static int summaryRequestId = 0;
 
@@ -414,10 +381,7 @@ int main(int argc, char** argv) {
       *serverResponse << "HTTP/1.1 200 OK\r\nAccess-Control-Allow-Origin: *\r\nContent-Type: application/json; charset=utf-8\r\nContent-Length: " << response.length() << "\r\n\r\n" << response;
       return;
     }
-
-    // prepare timer:
-    // TODO Shouldn't have to do this, a query is not a benchmark
-    calculator.algorithmCalculationTime.start();
+    Calculator calculator(transitData, *geoFilter);
 
     // prepare parameters:
     std::vector<std::pair<std::string, std::string>> parametersWithValues;
@@ -478,7 +442,7 @@ int main(int argc, char** argv) {
 
   // Routing request for a single origin destination
   // TODO Copy-pasted and adapted from /route/v1/transit. There's still a lot of common code. Application code should be extracted to common functions outside the web server
-  server.resource["^/v2/accessibility[/]?$"]["GET"]=[&server, &calculator, &dataStatus, &transitData](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> request) {
+  server.resource["^/v2/accessibility[/]?$"]["GET"]=[&server, &dataStatus, &transitData, &geoFilter](std::shared_ptr<HttpServer::Response> serverResponse, std::shared_ptr<HttpServer::Request> request) {
     // Have a global id to match the requests in the logs
     static int accessibilityRequestId = 0;
 
@@ -488,10 +452,7 @@ int main(int argc, char** argv) {
       *serverResponse << "HTTP/1.1 200 OK\r\nAccess-Control-Allow-Origin: *\r\nContent-Type: application/json; charset=utf-8\r\nContent-Length: " << response.length() << "\r\n\r\n" << response;
       return;
     }
-
-    // prepare timer:
-    // TODO Shouldn't have to do this, a query is not a benchmark
-    calculator.algorithmCalculationTime.start();
+    Calculator calculator(transitData, *geoFilter);
 
     // prepare parameters:
     std::vector<std::pair<std::string, std::string>> parametersWithValues;

diff --git a/tests/benchmark_csa/benchmark_CSA_test.cpp b/tests/benchmark_csa/benchmark_CSA_test.cpp
@@ -25,7 +25,6 @@ const int NB_ITER = 30;
 // Global test suite variables, they should not be reset for each test
 TransitData* transitData; //TODO Required only to get the scenario, we might want to get it in another way
 EuclideanGeoFilter* geoFilter;
-Calculator* calculator;
 std::ofstream benchmarkResultsFile;
 std::ofstream benchmarkDetailedResultsFile;
 
@@ -70,14 +69,10 @@ class BenchmarkCSATests : public ConstantBenchmarkCSATests
     // Use simple distance in the benchmark instead of OSRM
     geoFilter = new TrRouting::EuclideanGeoFilter();
 
-    calculator = new TrRouting::Calculator(*transitData, *geoFilter);
-
     if (!updateCalculatorFromCache(*transitData)) {
       ASSERT_EQ(true, false);
       return;
     }
-    //TODO is this necessary? We removed a call to prepare() and added this one
-    calculator->initializeCalculationData();
 
     // Prepare the result files
     time_t rawtime;
@@ -102,14 +97,10 @@ class BenchmarkCSATests : public ConstantBenchmarkCSATests
   {
     benchmarkResultsFile.close();
     benchmarkDetailedResultsFile.close();
-    delete calculator;
   }
 
   void benchmarkCurrentParams(TrRouting::RouteParameters &routeParams, bool expectResult, int nbIter)
   {
-    // TODO Shouldn't have to do this, a query is not a benchmark
-    calculator->algorithmCalculationTime.start();
-
     double results[nbIter];
     for (int i = 0; i < nbIter; i++)
     {
@@ -119,14 +110,16 @@ class BenchmarkCSATests : public ConstantBenchmarkCSATests
 
       if (routeParams.isWithAlternatives()) {
         try {
-          calculator->alternativesRouting(routeParams);
+          Calculator calculator(*transitData, *geoFilter);
+          calculator.alternativesRouting(routeParams);
           ASSERT_TRUE(expectResult);
         } catch (TrRouting::NoRoutingFoundException& e) {
           ASSERT_FALSE(expectResult);
         }
       } else {
         try {
-          calculator->calculateSingle(routeParams);
+          Calculator calculator(*transitData, *geoFilter);
+          calculator.calculateSingle(routeParams);
           ASSERT_TRUE(expectResult);
         } catch (TrRouting::NoRoutingFoundException& e) {
           ASSERT_FALSE(expectResult);

diff --git a/tests/connection_scan_algorithm/csa_access_map_test.cpp b/tests/connection_scan_algorithm/csa_access_map_test.cpp
@@ -32,8 +32,7 @@ void AccessMapFixtureTests::SetUp()
 
 std::unique_ptr<TrRouting::AllNodesResult> AccessMapFixtureTests::calculateOd(TrRouting::AccessibilityParameters& parameters)
 {
-    // TODO Shouldn't need to do this, but we do for now, benchmark needs to be started
-    calculator.algorithmCalculationTime.start();
+    TrRouting::Calculator calculator(transitData, geoFilter);
     return calculator.calculateAllNodes(parameters);
 }
 

diff --git a/tests/connection_scan_algorithm/csa_route_calculation_test.cpp b/tests/connection_scan_algorithm/csa_route_calculation_test.cpp
@@ -537,7 +537,6 @@ TEST_F(SingleRouteCalculationFixtureTests, NoRoutingTravelTimeLongerTrip)
 
 std::unique_ptr<TrRouting::RoutingResult> SingleRouteCalculationFixtureTests::calculateOd(TrRouting::RouteParameters& parameters)
 {
-    // TODO Shouldn't need to do this, but we do for now, benchmark needs to be started
-    calculator.algorithmCalculationTime.start();
+    TrRouting::Calculator calculator(transitData, geoFilter);
     return calculator.calculateSingle(parameters);
 }
diff --git a/tests/connection_scan_algorithm/csa_route_transfer_alternatives_test.cpp b/tests/connection_scan_algorithm/csa_route_transfer_alternatives_test.cpp
@@ -352,9 +352,6 @@ TEST_F(SingleTAndACalculationFixtureTests, TripWithNoRoutingAlternatives)
 
 TrRouting::AlternativesResult SingleTAndACalculationFixtureTests::calculateWithAlternatives(TrRouting::RouteParameters& parameters)
 {
-    // TODO Shouldn't need to do this, but we do for now, benchmark needs to be started
-    calculator.algorithmCalculationTime.start();
-
+    TrRouting::Calculator calculator(transitData, geoFilter);
     return calculator.alternativesRouting(parameters);
-
 }
diff --git a/tests/connection_scan_algorithm/csa_test_base.cpp b/tests/connection_scan_algorithm/csa_test_base.cpp
@@ -48,7 +48,6 @@ void BaseCsaFixtureTests::SetUp() {
     // Enable full debug output in the test runs
     spdlog::set_level(spdlog::level::debug);
 
-    calculator.initializeCalculationData();
 }
 
 void BaseCsaFixtureTests::assertNoRouting(const TrRouting::NoRoutingFoundException& exception, TrRouting::NoRoutingReason expectedReason)

diff --git a/tests/connection_scan_algorithm/csa_test_base.hpp b/tests/connection_scan_algorithm/csa_test_base.hpp
@@ -7,7 +7,6 @@
 #include "csa_test_data_fetcher.hpp"
 #include "transit_data.hpp"
 #include "euclideangeofilter.hpp"
-#include "calculator.hpp"
 #include "routing_result.hpp"
 
 
@@ -21,11 +20,9 @@ class BaseCsaFixtureTests : public ::testing::Test
     TestDataFetcher dataFetcher;
     TrRouting::TransitData transitData;
     TrRouting::EuclideanGeoFilter geoFilter;
-    // Calculator is the entry point to run the algorithm, it is part of the test object since (for now) there is nothing specific for its initialization.
-    TrRouting::Calculator calculator;
 
 public:
-    BaseCsaFixtureTests() : dataFetcher(TestDataFetcher()), transitData(TrRouting::TransitData(dataFetcher)), calculator(TrRouting::Calculator(transitData, geoFilter)) {}
+    BaseCsaFixtureTests() : dataFetcher(TestDataFetcher()), transitData(TrRouting::TransitData(dataFetcher)) {}
     void SetUp();
     // Assert the result returned an exception and that the reason matches the expected reason
     void assertNoRouting(const TrRouting::NoRoutingFoundException& exception, TrRouting::NoRoutingReason expectedReason);