-
Notifications
You must be signed in to change notification settings - Fork 4
Add utility print functions #395
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 #395 +/- ##
==========================================
+ Coverage 85.09% 85.13% +0.03%
==========================================
Files 53 53
Lines 5763 5784 +21
Branches 649 649
==========================================
+ Hits 4904 4924 +20
- Misses 848 849 +1
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:
|
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
Adds stream-based “print/describe” utilities for mobility components and validates them via new tests.
Changes:
- Add
RoadNetwork::describe(std::ostream&)for emitting a human-readable network description. - Add
RoadDynamics::summary(std::ostream&)plus new agent lifecycle counters and hook them into the dynamics. - Expose
describe()/summary()via pybind11 and add doctest coverage asserting expected output substrings.
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 |
|---|---|
| test/mobility/Test_graph.cpp | Adds doctest assertions for RoadNetwork::describe() output. |
| test/mobility/Test_dynamics.cpp | Adds doctest assertions for FirstOrderDynamics::summary() output. |
| src/dsf/mobility/RoadNetwork.hpp | Declares describe() and adjusts count return types to std::size_t. |
| src/dsf/mobility/RoadNetwork.cpp | Implements RoadNetwork::describe(). |
| src/dsf/mobility/RoadDynamics.hpp | Adds counters + summary() and increments counters at runtime. |
| src/dsf/bindings.cpp | Adds Python bindings for RoadNetwork.describe() and FirstOrderDynamics.summary(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/dsf/mobility/RoadDynamics.hpp
Outdated
| std::size_t m_nAgents{0}, m_nAddedAgents{0}, m_nInsertedAgents{0}, m_nKilledAgents{0}, | ||
| m_nArrivedAgents{0}; |
Copilot
AI
Jan 23, 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.
These counters are updated from within TBB-parallel regions (e.g., m_evolveStreet() is called inside tbb::parallel_for), but they are plain std::size_t. Increment/decrement from multiple threads will cause data races/UB and make summary() unreliable. Use atomics (e.g., std::atomic<std::size_t>) or thread-local reduction (e.g., tbb::combinable) and consolidate after the parallel section.
| this->m_killAgent(pStreet->dequeue(queueIndex)); | ||
| ++m_nKilledAgents; | ||
| continue; |
Copilot
AI
Jan 23, 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.
m_evolveStreet() runs under tbb::parallel_for (see evolve()), so this increment is not thread-safe on a plain std::size_t counter and can race. Make the counter atomic or accumulate per-thread and reduce after the parallel region.
| auto pAgent = this->m_killAgent(pStreet->dequeue(queueIndex)); | ||
| ++m_nArrivedAgents; | ||
| if (reinsert_agents) { |
Copilot
AI
Jan 23, 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.
m_evolveStreet() is executed in parallel; incrementing m_nArrivedAgents here can race (plain std::size_t). Use an atomic counter or a per-thread accumulator reduced after the parallel region.
| /// @brief Print a summary of the dynamics to an output stream | ||
| /// @param os The output stream to write to (default is std::cout) | ||
| /// @details The summary includes: |
Copilot
AI
Jan 23, 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.
summary() uses std::ostream and defaults the argument to std::cout, but this header doesn’t include <ostream>/<iostream> directly (it currently relies on transitive includes via RoadNetwork.hpp). Add the appropriate standard header include(s) here to avoid fragile build dependencies.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.