Refactor TrafficLights non-local optimization + add utility functions #301
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the TrafficLight cycle logic to optimize non‐local phase adjustments, and adds new utility functions for traffic light and road network management.
- Refactored isGreen() implementation in TrafficLightCycle for handling cycle wraparound
- Introduced increasePhases() in TrafficLight to update cycle phases
- Added importTrafficLights() in RoadNetwork to load traffic light configurations from a file
- Updated related header declarations and modified agent queue and optimization handling in RoadDynamics
- Bumped DSM version from 2.6.25 to 2.6.26
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/dsm/sources/TrafficLight.cpp | Updated isGreen() logic and added increasePhases() implementation |
| src/dsm/sources/RoadNetwork.cpp | Improved traffic light auto-init logic and added importTrafficLights() |
| src/dsm/headers/TrafficLight.hpp | Declared new increasePhases() method |
| src/dsm/headers/RoadNetwork.hpp | Declared new importTrafficLights() method |
| src/dsm/headers/RoadDynamics.hpp | Enhanced agent street-id checks and optimized double tail logic |
| src/dsm/dsm.hpp | Updated DSM version patch number |
Comments suppressed due to low confidence (3)
src/dsm/sources/TrafficLight.cpp:140
- [nitpick] The parameter 'phase' in increasePhases() could be more descriptive (e.g., 'phaseOffset') to clarify its purpose in adjusting the current phase.
void TrafficLight::increasePhases(Delay const phase) {
src/dsm/sources/RoadNetwork.cpp:964
- [nitpick] The storedGreenTimes map is declared inside the loop, which means its state is reset for each line. If multiple entries for the same traffic light are expected, consider declaring it outside the loop or clarifying the expected file format.
std::unordered_map<Id, Delay> storedGreenTimes;
src/dsm/headers/RoadDynamics.hpp:1265
- Ensure that using pStreet->id() as the key in m_queuesAtTrafficLights is consistent with how keys are initialized elsewhere, to avoid mismatches in queue lookups.
if (!m_queuesAtTrafficLights.contains(pStreet->id())) {
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #301 +/- ##
==========================================
- Coverage 85.57% 84.78% -0.79%
==========================================
Files 38 38
Lines 5503 5574 +71
Branches 565 572 +7
==========================================
+ Hits 4709 4726 +17
- Misses 794 848 +54
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cppcheck (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors traffic light cycle logic, adds utility functions for phase adjustments, implements CSV-based traffic light import, extends node export with new columns, migrates queue data structures to TBB concurrent containers, and enhances the double-tail optimization in road dynamics.
- Simplify and correct wraparound logic in
TrafficLightCycle::isGreenand addincreasePhases - Introduce
importTrafficLightsand updateexportNodesformat inRoadNetwork - Switch to
tbb::concurrent_unordered_mapinRoadDynamics, fix agent street‐ID checks, and improve optimization flow - Bump DSM patch version to 2.6.26
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dsm/sources/TrafficLight.cpp | Rewrite isGreen, add increasePhases implementation |
| src/dsm/headers/TrafficLight.hpp | Declared increasePhases method |
| src/dsm/sources/RoadNetwork.cpp | Added importTrafficLights, enhanced auto-init, updated CSV export |
| src/dsm/headers/RoadNetwork.hpp | Declared importTrafficLights in header |
| src/dsm/headers/RoadDynamics.hpp | Migrated m_queuesAtTrafficLights to TBB, adjusted agent and queue logic |
| src/dsm/sources/RoadDynamics.cpp | Fixed ID misuse, removed capacity guard in loop, added zero‐division risk |
| src/dsm/dsm.hpp | Incremented patch version |
Comments suppressed due to low confidence (4)
src/dsm/sources/RoadDynamics.cpp:834
- This message logs missing
streetIdbut the check is onnextStreetId; consider updating the error text to referencenextStreetIdfor accuracy.
Logger::error(std::format("Agent {} has no street id", pAgentTemp->id()));
src/dsm/sources/RoadDynamics.cpp:1841
- Potential divide-by-zero if
nis zero (no incoming traffic-light nodes found); add a checkif (n > 0)before performing the division.
density /= n;
src/dsm/headers/TrafficLight.hpp:138
- [nitpick] The parameter name
phaseis ambiguous against the memberm_phase; consider renaming todeltaPhaseoroffsetfor clarity.
void increasePhases(Delay const phase);
src/dsm/sources/RoadDynamics.cpp:1053
- Removing the capacity check may cause an infinite loop if all edges are full; reintroduce a guard or fallback to avoid potential hangs.
} while (this->graph().edge(streetId)->isFull());
src/dsm/headers/TrafficLight.hpp
Outdated
| /// @brief Decrease the green times of the traffic light for priority streets and increase the green times for non-priority streets | ||
| /// @param delta Delay, the time to increase or decrease the green times | ||
| void decreaseGreenTimes(Delay const delta); | ||
|
|
There was a problem hiding this comment.
Add a brief Doxygen comment above increasePhases explaining its purpose and parameters to keep the public API documented.
| /// @brief Increase the phase times of the traffic light cycles | |
| /// @param phase Delay, the amount of time to increase the phase for each cycle |
src/dsm/sources/RoadNetwork.cpp
Outdated
| while (!file.eof()) { | ||
| std::getline(file, line); | ||
| if (line.empty()) { | ||
| continue; | ||
| } | ||
| std::unordered_map<Id, Delay> storedGreenTimes; |
There was a problem hiding this comment.
storedGreenTimes is reinitialized on every loop iteration, preventing multi-line grouping; move its declaration outside the while to accumulate across all lines.
| while (!file.eof()) { | |
| std::getline(file, line); | |
| if (line.empty()) { | |
| continue; | |
| } | |
| std::unordered_map<Id, Delay> storedGreenTimes; | |
| std::unordered_map<Id, Delay> storedGreenTimes; | |
| while (!file.eof()) { | |
| std::getline(file, line); | |
| if (line.empty()) { | |
| continue; | |
| } |
src/dsm/sources/RoadNetwork.cpp
Outdated
| while (!file.eof()) { | ||
| std::getline(file, line); |
There was a problem hiding this comment.
Using while (!file.eof()) is error-prone; prefer while (std::getline(file, line)) to handle file input more robustly.
| while (!file.eof()) { | |
| std::getline(file, line); | |
| while (std::getline(file, line)) { |
| std::ofstream file{path}; | ||
| // Column names | ||
| file << "id;source_id;target_id;name;coil_code;geometry\n"; | ||
| file << "id;source_id;target_id;length;capacity;name;coil_code;geometry\n"; |
There was a problem hiding this comment.
The CSV header was extended to include length and capacity; update the function comment above exportNodes in the header to reflect the new columns.
src/dsm/headers/RoadNetwork.hpp
Outdated
| /// - forbiddenTurns: The forbidden turns of the street, encoding information about street into which the street cannot output agents. The format is a string "sourceId1-targetid1, sourceId2-targetid2,..." | ||
| /// - coilcode: An integer code to identify the coil located on the street | ||
| void importOSMEdges(const std::string& fileName); | ||
|
|
There was a problem hiding this comment.
Add a Doxygen comment describing importTrafficLights, its expected file format, and behavior on parsing errors.
| /** | |
| * @brief Import traffic light data from a file. | |
| * @param fileName The path to the file containing traffic light data. | |
| * @details The file format is csv-like with the ';' separator. Supported columns (in order): | |
| * - id: The unique identifier of the traffic light | |
| * - junctionId: The id of the junction where the traffic light is located | |
| * - state: The initial state of the traffic light (e.g., "red", "green", "yellow") | |
| * - timing: The timing configuration for the traffic light, in seconds | |
| * | |
| * Parsing errors, such as missing or malformed columns, will be logged as warnings, | |
| * and the corresponding traffic light entry will be skipped. | |
| */ |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the traffic light optimization to support a non-local “green-wave” approach and adds helper methods for phase adjustment and CSV import/export enhancements.
- Simplified
isGreenlogic and addedincreasePhasestoTrafficLight - Normalized and sorted street angles, plus a new
importTrafficLightsCSV loader - Updated
optimizeTrafficLightsto include ratio-based non-local optimization and bumped DSM version
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dsm/sources/TrafficLight.cpp | Refactored isGreen logic; added increasePhases |
| src/dsm/headers/TrafficLight.hpp | Declared increasePhases |
| src/dsm/sources/RoadNetwork.cpp | Normalized/ordered street angles; CSV import/export |
| src/dsm/headers/RoadNetwork.hpp | Added importTrafficLights signature and docs |
| src/dsm/headers/RoadDynamics.hpp | Switched to tbb::concurrent_unordered_map; updated optimizer signature and logic |
| src/dsm/sources/RoadDynamics.cpp | Adapted non-local optimization and queue tracking |
| src/dsm/dsm.hpp | Bumped version minor/patch to 27.0 |
| examples/slow_charge_tl.cpp | Updated optimizeTrafficLights invocation to new signature |
Comments suppressed due to low confidence (3)
src/dsm/sources/RoadDynamics.cpp:835
- This check logs an error for missing
streetIdbut then continues to usenextStreetId; either log andcontinueor checknextStreetIdbefore dereferencing to avoid invalid state.
if (!pAgentTemp->streetId().has_value()) {
src/dsm/sources/RoadNetwork.cpp:1055
- The original capacity check (
&& this->nAgents() < this->graph().maxCapacity()) was removed, so this loop may never terminate if all edges are full; reintroduce a guard against exceeding max capacity.
} while (this->graph().edge(streetId)->isFull());
src/dsm/sources/TrafficLight.cpp:140
- [nitpick] The new
increasePhasesmethod does not appear to have accompanying unit tests; consider adding tests to verify phase updates for both wraparound and non-wraparound cases.
void TrafficLight::increasePhases(Delay const phase) {
| std::vector<std::pair<Id, double>> sortedAngles; | ||
| std::copy( | ||
| streetAngles.begin(), streetAngles.end(), std::back_inserter(sortedAngles)); | ||
| std::sort(sortedAngles.begin(), | ||
| sortedAngles.end(), | ||
| [](auto const& a, auto const& b) { return a.second < b.second; }); | ||
| streetAngles.clear(); | ||
| for (auto const& [streetId, angle] : sortedAngles) { | ||
| streetAngles.emplace(streetId, angle); | ||
| } |
There was a problem hiding this comment.
[nitpick] Reinserting sorted entries into an unordered_map does not guarantee stable iteration order; if ordering matters later, consider using a vector or an ordered map structure instead.
| std::vector<std::pair<Id, double>> sortedAngles; | |
| std::copy( | |
| streetAngles.begin(), streetAngles.end(), std::back_inserter(sortedAngles)); | |
| std::sort(sortedAngles.begin(), | |
| sortedAngles.end(), | |
| [](auto const& a, auto const& b) { return a.second < b.second; }); | |
| streetAngles.clear(); | |
| for (auto const& [streetId, angle] : sortedAngles) { | |
| streetAngles.emplace(streetId, angle); | |
| } | |
| std::sort(streetAngles.begin(), | |
| streetAngles.end(), | |
| [](auto const& a, auto const& b) { return a.second < b.second; }); |
Co-authored-by: Copilot <[email protected]>
|
It is not clear why macos examples fail... I don't really care about mac at all |
No description provided.