-
Notifications
You must be signed in to change notification settings - Fork 4
Add stationary probability for random vehicles movement #376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #376 +/- ##
==========================================
- Coverage 83.27% 83.24% -0.04%
==========================================
Files 53 53
Lines 5275 5330 +55
Branches 606 608 +2
==========================================
+ Hits 4393 4437 +44
- Misses 871 882 +11
Partials 11 11
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:
|
| // Street 0: 0 -> 1 | ||
| // High length to hold all agents, high transport capacity to move them all at once | ||
| network.addStreet(Street(0, | ||
| {0, 1}, |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
| static_cast<double>(highCapacity))); | ||
| // Street 1: 1 -> 2 | ||
| network.addStreet(Street(1, | ||
| {1, 2}, |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
| static_cast<double>(highCapacity))); | ||
| // Street 2: 1 -> 3 | ||
| network.addStreet(Street(2, | ||
| {1, 3}, |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
| // Ratio 1:2 -> P(1) = 1/3, P(2) = 2/3 | ||
|
|
||
| double ratio = | ||
| (countStreet1 > 0) ? static_cast<double>(countStreet2) / countStreet1 : 0.0; |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 10.4 rule Note test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements stationary weight functionality for streets in the traffic simulation framework, allowing users to bias random agent navigation toward certain streets by assigning different stationary probability weights.
Key Changes
- Added
m_stationaryWeightmember (default 1.0) to theStreetclass with getter/setter methods - Modified the transition probability calculation in
RoadDynamicsto incorporate the square root of the weight ratio between current and next streets - Added
setStreetStationaryWeights()method toRoadNetworkfor bulk setting of street weights - Added Python bindings for the new functionality and exposed the
graph()method onFirstOrderDynamics - Included a comprehensive test case demonstrating that agents distribute according to the expected 2:1 ratio when streets have weights of 4.0 and 1.0
- Removed commented-out transition probability code that was replaced by this implementation
- Declared copy constructor/assignment as deleted and move constructor/assignment as defaulted for
RoadNetwork
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dsf/mobility/Street.hpp | Added m_stationaryWeight member variable with setter/getter methods; removed old commented transition probability code |
| src/dsf/mobility/RoadNetwork.hpp | Added setStreetStationaryWeights() method declaration; added copy/move semantics declarations |
| src/dsf/mobility/RoadNetwork.cpp | Implemented setStreetStationaryWeights() using parallel execution; removed old commented JSON parsing code for transition probabilities |
| src/dsf/mobility/RoadDynamics.hpp | Modified probability calculation to include sqrt(weightRatio) factor; added stationaryWeightCurrent and stationaryWeightNext variables |
| src/dsf/bindings.cpp | Added Python bindings for setStreetStationaryWeights() and graph() methods |
| test/mobility/Test_dynamics.cpp | Added new test case validating stationary weight impact on agent distribution with 3000 agents |
| src/dsf/dsf.hpp | Incremented patch version from 9 to 10 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/dsf/mobility/Street.hpp
Outdated
| // }; | ||
| /// @brief Set the street's stationary weight | ||
| /// @param weight The street's stationary weight | ||
| inline void setStationaryWeight(double const weight) { m_stationaryWeight = weight; } |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing input validation for the weight parameter. Negative or zero weights could lead to incorrect probability calculations or mathematical errors (e.g., when taking square roots in RoadDynamics.hpp line 558). Consider adding validation to ensure weights are positive, or document if non-positive values are intentionally allowed.
| inline void setStationaryWeight(double const weight) { m_stationaryWeight = weight; } | |
| inline void setStationaryWeight(double const weight) { | |
| if (weight <= 0.0) { | |
| throw std::invalid_argument("Stationary weight must be positive."); | |
| } | |
| m_stationaryWeight = weight; | |
| } |
| // P(1) ~ speed * speed * sqrt(1/1) = 100 | ||
| // P(2) ~ speed * speed * sqrt(4/1) = 200 |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test comment contains an error in the probability calculation. The comment states:
// P(1) ~ speed * speed * sqrt(1/1) = 100
// P(2) ~ speed * speed * sqrt(4/1) = 200
However, based on the actual formula in RoadDynamics.hpp line 558 (probability = speedCurrent * speedNext * std::sqrt(weightRatio)), the calculation should be:
- P(1) = speedCurrent (10.0) × speedNext (10.0) × sqrt(1.0/1.0) = 100
- P(2) = speedCurrent (10.0) × speedNext (10.0) × sqrt(4.0/1.0) = 200
The formulas are actually correct, but stating "speed * speed" is misleading since it's actually speedCurrent × speedNext (which happen to be the same value in this test). Consider clarifying this as "speedCurrent * speedNext" for accuracy.
| // P(1) ~ speed * speed * sqrt(1/1) = 100 | |
| // P(2) ~ speed * speed * sqrt(4/1) = 200 | |
| // P(1) ~ speedCurrent * speedNext * sqrt(1/1) = 10.0 * 10.0 * 1 = 100 | |
| // P(2) ~ speedCurrent * speedNext * sqrt(4/1) = 10.0 * 10.0 * 2 = 200 |
| std::cout << "Agents on Street 1: " << countStreet1 << std::endl; | ||
| std::cout << "Agents on Street 2: " << countStreet2 << std::endl; | ||
| std::cout << "Ratio (Street 2 / Street 1): " << ratio << std::endl; | ||
|
|
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug output statements (std::cout) should be removed from test code or replaced with proper logging. These print statements were likely used during development and should not remain in production test code as they clutter test output.
| std::cout << "Agents on Street 1: " << countStreet1 << std::endl; | |
| std::cout << "Agents on Street 2: " << countStreet2 << std::endl; | |
| std::cout << "Ratio (Street 2 / Street 1): " << ratio << std::endl; |
| .def("graph", | ||
| &dsf::mobility::FirstOrderDynamics::graph, | ||
| pybind11::return_value_policy::reference_internal, | ||
| dsf::g_docstrings.at("dsf::Dynamics::graph").c_str()) |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Python bindings reference dsf::Dynamics::graph in the docstrings map (line 542), but there's no documentation for a non-const graph() method in the Dynamics class. The base Dynamics class only has a const version (inline const auto& graph() const). This will cause a runtime error when the docstrings are loaded, as the key won't exist in the generated docstrings map. Either:
- Add documentation for the non-const
graph()method if it exists - Use the correct docstring key if this method has different documentation
- Remove the docstring lookup if this is meant to reuse the const version's documentation
| dsf::g_docstrings.at("dsf::Dynamics::graph").c_str()) | |
| dsf::g_docstrings.at("dsf::Dynamics::graph() const").c_str()) |
| .def("setStreetStationaryWeights", | ||
| &dsf::mobility::RoadNetwork::setStreetStationaryWeights, | ||
| pybind11::arg("weights"), | ||
| dsf::g_docstrings.at("dsf::mobility::RoadNetwork::setStreetStationaryWeights") | ||
| .c_str()) |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Python bindings reference dsf::mobility::RoadNetwork::setStreetStationaryWeights in the docstrings map (line 178), but this method has no documentation in RoadNetwork.hpp (line 194). This will cause a runtime error when loading the Python module, as the docstrings are generated from Doxygen comments and this key won't exist in the g_docstrings map. Add proper Doxygen documentation for this public API method.
| double probability = speedCurrent * speedNext; | ||
| double const stationaryWeightNext = pStreetOut->stationaryWeight(); | ||
| auto const weightRatio{stationaryWeightNext / | ||
| stationaryWeightCurrent}; // SQRT (p_i / p_j) |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment incorrectly states "SQRT (p_i / p_j)" but the actual calculation uses sqrt(weightRatio) where weightRatio = stationaryWeightNext / stationaryWeightCurrent. This suggests the comment should either say "sqrt(p_next / p_current)" or clarify what p_i and p_j represent in this context. The indices i and j are ambiguous here.
| stationaryWeightCurrent}; // SQRT (p_i / p_j) | |
| stationaryWeightCurrent}; // sqrt(p_next / p_current) |
| /// @brief Set the street's stationary weight | ||
| /// @param weight The street's stationary weight | ||
| inline void setStationaryWeight(double const weight) { | ||
| weight > 0. ? m_stationaryWeight = weight |
Check warning
Code scanning / Cppcheck (reported by Codacy)
AST broken, ternary operator missing operand(s) Warning
No description provided.