[Experimental] Add support for real-time transition probabilities #368
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #368 +/- ##
==========================================
+ Coverage 83.54% 83.56% +0.02%
==========================================
Files 51 51
Lines 5081 5216 +135
Branches 584 602 +18
==========================================
+ Hits 4245 4359 +114
- Misses 824 845 +21
Partials 12 12
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 adds support for real-time transition probabilities for random agents in traffic simulations. Random agents now select their next street based on speed-weighted probabilities that dynamically account for traffic density, with a penalty for U-turns. This replaces purely random selection with a more realistic traffic flow model.
Key changes:
- Introduced dynamic transition probability calculation based on street speeds and density
- Added
addRandomAgentsmethods for spawning agents with configurable origin weights - Modified
Agent::setStreetIdAPI to enforce stricter state management - Changed
m_nextStreetIdto returnstd::optional<Id>to handle cases with no valid moves
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/mobility/Test_dynamics.cpp | Added tests for transition probability distribution with deterministic random seeds |
| test/mobility/Test_agent.cpp | Updated tests to reflect stricter setStreetId API requirements |
| src/dsf/mobility/Street.hpp | Added transition probabilities member and accessor methods |
| src/dsf/mobility/RoadNetwork.hpp | Removed unused bCreateInverse parameter from JSON importer |
| src/dsf/mobility/RoadNetwork.cpp | Removed inverse edge creation logic, added commented transition probability parsing |
| src/dsf/mobility/RoadDynamics.hpp | Implemented speed-based transition probabilities and new agent spawning methods |
| src/dsf/mobility/FirstOrderDynamics.hpp | Made m_speedFactor virtual to support transition probability calculations |
| src/dsf/mobility/Agent.hpp | Inlined setNextStreetId and updated formatter to use isRandom() |
| src/dsf/mobility/Agent.cpp | Enhanced setStreetId with validation logic to prevent invalid state transitions |
| src/dsf/dsf.hpp | Bumped patch version from 4.4.2 to 4.4.3 |
| src/dsf/bindings.cpp | Added Python bindings for addRandomAgents methods |
| README.md | Added --output-on-failure flag to ctest command for better debugging |
Comments suppressed due to low confidence (1)
src/dsf/mobility/RoadDynamics.hpp:564
- Potential runtime error for random agents: At line 564,
pAgent->itineraryId()is called without checking if the agent is random. According to the implementation in Agent.cpp,itineraryId()throws astd::logic_errorwhen called on a random agent (whenisRandom()returns true). The code at line 560 only checks if!this->itineraries().empty(), but doesn't verify that the agent is not random before callingitineraryId().
Add a check to ensure the agent is not random before attempting to access its itinerary:
if (!pAgent->isRandom() && !this->itineraries().empty()) { if (!this->itineraries().empty()) {
std::uniform_real_distribution<double> uniformDist{0., 1.};
if (!(m_errorProbability.has_value() &&
uniformDist(this->m_generator) < m_errorProbability)) {
const auto& it = this->itineraries().at(pAgent->itineraryId());
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (randValue <= cumulativeProbability) { | ||
| return targetStreetId; | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] Missing fallback return: If the random selection loop (lines 510-515) doesn't find a matching street (which could happen due to floating-point precision issues), the function will continue to the default logic at line 530. However, this could be unexpected behavior. Consider adding an explicit fallback or assertion after the loop to make the intent clear, or add a comment explaining why falling through is the desired behavior.
| } | |
| } | |
| // If we reach here, something went wrong due to floating-point precision. | |
| assert(false && "Unreachable: random selection failed due to floating-point precision issues."); |
src/dsf/mobility/RoadDynamics.hpp
Outdated
| std::uniform_real_distribution<double> uniformDist{0., 1.}; | ||
| auto const bUniformSpawn{spawnWeights.empty()}; | ||
| auto const bSingleSource{spawnWeights.size() == 1}; | ||
| while (--nAgents) { |
There was a problem hiding this comment.
Off-by-one error: This loop will only add nAgents - 1 agents instead of nAgents agents. The loop condition should be while (nAgents--) (postfix decrement) or the loop should be while (--nAgents > 0), or better yet, use a for loop: for (std::size_t i = 0; i < nAgents; ++i).
Currently, if nAgents = 3, the loop executes:
- First iteration:
--nAgentsmakes it 2 (truthy, executes) - Second iteration:
--nAgentsmakes it 1 (truthy, executes) - Third iteration:
--nAgentsmakes it 0 (falsy, stops)
So only 2 agents are added in the loop, plus 1 agent added after the loop (lines 1362-1370), for a total of 3. However, the duplicated code after the loop (lines 1362-1370) appears to be unintentional.
src/dsf/mobility/RoadDynamics.hpp
Outdated
| auto const randValue{uniformDist(this->m_generator)}; | ||
| double cumulativeWeight{0.}; | ||
| for (auto const& [spawnNodeId, weight] : spawnWeights) { | ||
| cumulativeWeight += weight; | ||
| if (randValue <= cumulativeWeight) { | ||
| this->addAgent(std::nullopt, spawnNodeId); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicated code: Lines 1362-1370 duplicate the agent spawning logic that should have been handled in the loop above. This appears to be a workaround for the off-by-one error in line 1344. Instead of having this duplicated code, fix the loop condition to properly iterate nAgents times and remove this duplicated block.
| /// @brief Check if the street is full | ||
| /// @return bool, True if the street is full, false otherwise | ||
| inline bool isFull() const final { return this->nAgents() == this->m_capacity; } | ||
|
|
There was a problem hiding this comment.
Missing documentation for the return type: The getter method for transition probabilities is missing a @brief documentation comment and return type description. Add documentation following the pattern used in the codebase:
/// @brief Get the street's transition probabilities
/// @return const std::unordered_map<Id, double>& The street's transition probabilities
inline auto const& transitionProbabilities() const {
return m_transitionProbabilities;
}| /// @brief Get the street's transition probabilities | |
| /// @return const std::unordered_map<Id, double>& The street's transition probabilities |
src/dsf/mobility/RoadDynamics.hpp
Outdated
| for (const auto outEdgeId : outgoingEdges) { | ||
| auto const& pStreetOut{this->graph().edge(outEdgeId)}; | ||
| auto const speed{pStreetOut->maxSpeed() * | ||
| this->m_speedFactor(pStreetOut->density())}; | ||
| transitionProbabilities[pStreetOut->id()] = speed / speedCurrent; | ||
| if (pStreetOut->target() == pStreetCurrent->source()) { | ||
| transitionProbabilities[pStreetOut->id()] *= 0.1; // Discourage U-TURNS | ||
| } | ||
| cumulativeProbability += transitionProbabilities.at(pStreetOut->id()); |
There was a problem hiding this comment.
Potential division by zero: If speedCurrent is zero (which can happen if pStreetCurrent->maxSpeed() or this->m_speedFactor(pStreetCurrent->density()) is zero), the division at line 501 will result in undefined behavior. Add a check to handle this edge case:
if (speedCurrent == 0.0) {
// Fall through to default random selection logic
} else {
// Current logic
}or handle it by using uniform probabilities when speed is zero.
| for (const auto outEdgeId : outgoingEdges) { | |
| auto const& pStreetOut{this->graph().edge(outEdgeId)}; | |
| auto const speed{pStreetOut->maxSpeed() * | |
| this->m_speedFactor(pStreetOut->density())}; | |
| transitionProbabilities[pStreetOut->id()] = speed / speedCurrent; | |
| if (pStreetOut->target() == pStreetCurrent->source()) { | |
| transitionProbabilities[pStreetOut->id()] *= 0.1; // Discourage U-TURNS | |
| } | |
| cumulativeProbability += transitionProbabilities.at(pStreetOut->id()); | |
| if (speedCurrent == 0.0) { | |
| // Use uniform probabilities if speedCurrent is zero | |
| double uniformProb = 1.0; | |
| for (const auto outEdgeId : outgoingEdges) { | |
| transitionProbabilities[outEdgeId] = uniformProb; | |
| cumulativeProbability += uniformProb; | |
| } | |
| } else { | |
| for (const auto outEdgeId : outgoingEdges) { | |
| auto const& pStreetOut{this->graph().edge(outEdgeId)}; | |
| auto const speed{pStreetOut->maxSpeed() * | |
| this->m_speedFactor(pStreetOut->density())}; | |
| transitionProbabilities[pStreetOut->id()] = speed / speedCurrent; | |
| if (pStreetOut->target() == pStreetCurrent->source()) { | |
| transitionProbabilities[pStreetOut->id()] *= 0.1; // Discourage U-TURNS | |
| } | |
| cumulativeProbability += transitionProbabilities.at(pStreetOut->id()); | |
| } |
Grufoony
changed the title
No description provided.