-
Notifications
You must be signed in to change notification settings - Fork 4
Add Street::meanSpeed and Dynamics::saveStreetSpeeds functions
#399
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 #399 +/- ##
==========================================
+ Coverage 85.55% 85.82% +0.27%
==========================================
Files 53 54 +1
Lines 6064 6194 +130
Branches 657 671 +14
==========================================
+ Hits 5188 5316 +128
+ Misses 865 862 -3
- Partials 11 16 +5
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:
|
|
|
||
| for (const auto& entry : std::filesystem::directory_iterator(".")) { | ||
| if (entry.path().filename().string().find("street_speeds.csv") != | ||
| std::string::npos) { |
Check warning
Code scanning / Cppcheck (reported by Codacy)
Consider using std::find_if algorithm instead of a raw loop. Warning test
| street.dequeue(0, 10); | ||
|
|
||
| // Agent 1 added at time 0, exits at time 5 (speed = 100/5 = 20 m/s) | ||
| street.addAgent(std::make_unique<Agent>(1, 0, nullptr, 0), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
|
|
||
| WHEN("Agents exit at different times") { | ||
| // Agent 0 added at time 0, exits at time 10 (speed = 100/10 = 10 m/s) | ||
| street.addAgent(std::make_unique<Agent>(0, 0, nullptr, 0), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
| // Add agents at time 0 | ||
| street.addAgent(std::make_unique<Agent>(0, 0, nullptr, 0), 0); | ||
| street.addAgent(std::make_unique<Agent>(1, 0, nullptr, 0), 0); | ||
| street.addAgent(std::make_unique<Agent>(2, 0, nullptr, 0), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
| x2_mean += value * value; | ||
| }); | ||
| mean = x_mean / data.size(); | ||
| std = std::sqrt(x2_mean / data.size() - mean * mean); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.1 rule Note
| return; | ||
| } | ||
|
|
||
| std::for_each(data.begin(), data.end(), [&x_mean, &x2_mean](auto value) -> void { |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note
| if (data.empty()) { | ||
| mean = static_cast<T>(0); | ||
| std = static_cast<T>(0); | ||
| return; |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 15.5 rule Note
| template <typename TContainer> | ||
| Measurement(TContainer data) { | ||
| auto x_mean = static_cast<T>(0), x2_mean = static_cast<T>(0); | ||
| if (data.empty()) { |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 14.4 rule Note
src/dsf/utility/Measurement.hpp
Outdated
| Measurement(T mean, T std) : mean{mean}, std{std} {} | ||
| template <typename TContainer> | ||
| Measurement(TContainer data) { | ||
| auto x_mean = static_cast<T>(0), x2_mean = static_cast<T>(0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note
|
@filippodll Waiting for your approval |
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 PR introduces functionality to measure and track mean speeds of agents traversing streets in the mobility simulation framework. The changes add a Street::meanSpeed() method that calculates the average speed of agents based on their transit time through streets, and a Dynamics::saveStreetSpeeds() method to export this data to CSV files.
Changes:
- Added
Measurement<T>template struct to compute mean and standard deviation from data containers - Modified
Street::addAgent()andStreet::dequeue()to track agent insertion and exit times for speed calculation - Implemented
Street::meanSpeed()to compute average speeds with optional data reset - Added
RoadDynamics::saveStreetSpeeds()to export street speeds to CSV format with normalization option - Updated all callsites and tests to accommodate new function signatures
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dsf/utility/Measurement.hpp | New utility struct for computing statistical measurements (mean and standard deviation) |
| src/dsf/base/Dynamics.hpp | Moved Measurement struct to separate header file for reusability |
| src/dsf/mobility/Street.hpp | Added member variables and method signatures for speed tracking |
| src/dsf/mobility/Street.cpp | Implemented speed tracking logic in addAgent, dequeue, and new meanSpeed method |
| src/dsf/mobility/RoadDynamics.hpp | Added saveStreetSpeeds method and updated all callsites with new time parameters |
| src/dsf/bindings.cpp | Exposed saveStreetSpeeds to Python API |
| test/mobility/Test_street.cpp | Updated tests with new parameters and added comprehensive meanSpeed test cases |
| test/mobility/Test_dynamics.cpp | Added tests for saveStreetSpeeds functionality |
| benchmark/Bench_Street.cpp | Updated benchmark code with new function signatures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SUBCASE("meanSpeed") { | ||
| GIVEN("A street with multiple agents") { | ||
| Street street{1, std::make_pair(0, 1), 100.0, 20.0}; | ||
|
|
||
| WHEN("No agents have exited") { | ||
| THEN("meanSpeed returns zero mean and std") { | ||
| auto measurement = street.meanSpeed(false); | ||
| CHECK_EQ(measurement.mean, 0.); | ||
| CHECK_EQ(measurement.std, 0.); | ||
| } | ||
| } | ||
|
|
||
| WHEN("Agents are added and dequeued at different times") { | ||
| // Add agents at time 0 | ||
| street.addAgent(std::make_unique<Agent>(0, 0, nullptr, 0), 0); | ||
| street.addAgent(std::make_unique<Agent>(1, 0, nullptr, 0), 0); | ||
| street.addAgent(std::make_unique<Agent>(2, 0, nullptr, 0), 0); | ||
|
|
||
| // Enqueue them | ||
| street.enqueue(0); | ||
| street.enqueue(0); | ||
| street.enqueue(0); | ||
|
|
||
| // Dequeue at time 5 (speed = 100/5 = 20 m/s for each) | ||
| street.dequeue(0, 5); | ||
| street.dequeue(0, 5); | ||
| street.dequeue(0, 5); | ||
|
|
||
| THEN("meanSpeed returns correct mean and std without reset") { | ||
| auto measurement = street.meanSpeed(false); | ||
| CHECK_EQ(measurement.mean, 20.0); | ||
| CHECK_EQ(measurement.std, 0.0); | ||
| } | ||
|
|
||
| THEN("meanSpeed data is not cleared when bReset=false") { | ||
| street.meanSpeed(false); | ||
| auto measurement = street.meanSpeed(false); | ||
| CHECK_EQ(measurement.mean, 20.0); | ||
| } | ||
|
|
||
| THEN("meanSpeed data is cleared when bReset=true") { | ||
| street.meanSpeed(true); | ||
| auto measurement = street.meanSpeed(false); | ||
| CHECK_EQ(measurement.mean, 0.0); | ||
| CHECK_EQ(measurement.std, 0.0); | ||
| } | ||
| } | ||
|
|
||
| WHEN("Agents exit at different times") { | ||
| // Agent 0 added at time 0, exits at time 10 (speed = 100/10 = 10 m/s) | ||
| street.addAgent(std::make_unique<Agent>(0, 0, nullptr, 0), 0); | ||
| street.enqueue(0); | ||
| street.dequeue(0, 10); | ||
|
|
||
| // Agent 1 added at time 0, exits at time 5 (speed = 100/5 = 20 m/s) | ||
| street.addAgent(std::make_unique<Agent>(1, 0, nullptr, 0), 0); | ||
| street.enqueue(0); | ||
| street.dequeue(0, 5); | ||
|
|
||
| THEN("meanSpeed returns correct mean and std") { | ||
| auto measurement = street.meanSpeed(false); | ||
| CHECK_EQ(measurement.mean, 15.0); // (10 + 20) / 2 | ||
| CHECK_EQ(measurement.std, | ||
| doctest::Approx(5.0)); // sqrt(((10-15)^2 + (20-15)^2)/2) | ||
| } | ||
| } | ||
| } | ||
| } |
Copilot
AI
Feb 3, 2026
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 test case for edge condition: The tests do not cover the case where an agent is dequeued at the same time it was added (i.e., when currentTime equals the insertion time, resulting in a time difference of zero). This edge case would cause a division by zero in the speed calculation. Consider adding a test case to verify this scenario is handled correctly.
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.
Should not happen
| for (auto const& [streetId, pStreet] : this->graph().edges()) { | ||
| double speed{pStreet->meanSpeed(true).mean}; | ||
| if (bNormalized) { | ||
| speed /= pStreet->maxSpeed(); |
Copilot
AI
Feb 3, 2026
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.
Potential division by zero: If pStreet->maxSpeed() returns 0, the normalization will result in division by zero. Consider adding a check to handle this edge case, such as skipping normalization or using a default value when maxSpeed is zero.
| speed /= pStreet->maxSpeed(); | |
| const auto maxSpeed = pStreet->maxSpeed(); | |
| if (maxSpeed != 0.0) { | |
| speed /= maxSpeed; | |
| } |
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.
Should not happen
| x2_mean += value * value; | ||
| }); | ||
| mean = x_mean / data.size(); | ||
| std = std::sqrt(x2_mean / data.size() - mean * mean); |
Copilot
AI
Feb 3, 2026
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.
Potential numerical instability in standard deviation calculation: The formula std::sqrt(x2_mean / data.size() - mean * mean) (line 28) can suffer from numerical precision issues when the mean is large relative to the variance. This can result in taking the square root of a small negative number due to floating-point errors. Consider using a more numerically stable two-pass algorithm or Welford's online algorithm to compute the variance.
| m_avgSpeeds.push_back(m_length / | ||
| (currentTime - m_agentsInsertionTimes[pAgent->id()])); | ||
| m_agentsInsertionTimes.erase(pAgent->id()); |
Copilot
AI
Feb 3, 2026
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.
Potential division by zero: If an agent is dequeued at the same time it was added (currentTime == m_agentsInsertionTimes[pAgent->id()]), this will result in division by zero. Consider adding a check to handle this edge case, such as using a minimum time delta or skipping the speed calculation if the time difference is zero.
| m_avgSpeeds.push_back(m_length / | |
| (currentTime - m_agentsInsertionTimes[pAgent->id()])); | |
| m_agentsInsertionTimes.erase(pAgent->id()); | |
| auto insertionIt = m_agentsInsertionTimes.find(pAgent->id()); | |
| if (insertionIt != m_agentsInsertionTimes.end()) { | |
| auto const delta = currentTime - insertionIt->second; | |
| if (delta > 0) { | |
| m_avgSpeeds.push_back(m_length / delta); | |
| } else { | |
| spdlog::warn( | |
| "Non-positive travel time ({} s) for {} on {}; skipping speed calculation.", | |
| delta, *pAgent, *this); | |
| } | |
| m_agentsInsertionTimes.erase(insertionIt); | |
| } |
ab93e2e to
cb018b6
Compare
cb018b6 to
c12f66d
Compare
|
|
||
| Measurement(T mean, T std) : mean{mean}, std{std}, is_valid{true} {} | ||
| template <typename TContainer> | ||
| Measurement(TContainer const& data) { |
Check warning
Code scanning / Cppcheck (reported by Codacy)
Struct 'Measurement' has a constructor with 1 argument that is not explicit. Warning
No description provided.