Conversation
|
|
||
| SQLite::Statement schema(db, "PRAGMA table_info(agent_data)"); | ||
| std::set<std::string> columns; | ||
| while (schema.executeStep()) { |
| while (schema.executeStep()) { | ||
| columns.insert(schema.getColumn(1).getString()); | ||
| } | ||
| CHECK(columns.count("id") == 1); |
| columns.insert(schema.getColumn(1).getString()); | ||
| } | ||
| CHECK(columns.count("id") == 1); | ||
| CHECK(columns.count("simulation_id") == 1); |
| } | ||
| CHECK(columns.count("id") == 1); | ||
| CHECK(columns.count("simulation_id") == 1); | ||
| CHECK(columns.count("agent_id") == 1); |
| CHECK(columns.count("id") == 1); | ||
| CHECK(columns.count("simulation_id") == 1); | ||
| CHECK(columns.count("agent_id") == 1); | ||
| CHECK(columns.count("edge_id") == 1); |
| } | ||
| CHECK(agentColumns.count("id") == 1); | ||
| CHECK(agentColumns.count("simulation_id") == 1); | ||
| CHECK(agentColumns.count("agent_id") == 1); |
| CHECK(agentColumns.count("id") == 1); | ||
| CHECK(agentColumns.count("simulation_id") == 1); | ||
| CHECK(agentColumns.count("agent_id") == 1); | ||
| CHECK(agentColumns.count("edge_id") == 1); |
| CHECK(agentColumns.count("simulation_id") == 1); | ||
| CHECK(agentColumns.count("agent_id") == 1); | ||
| CHECK(agentColumns.count("edge_id") == 1); | ||
| CHECK(agentColumns.count("time_step_in") == 1); |
| CHECK(agentColumns.count("agent_id") == 1); | ||
| CHECK(agentColumns.count("edge_id") == 1); | ||
| CHECK(agentColumns.count("time_step_in") == 1); | ||
| CHECK(agentColumns.count("time_step_out") == 1); |
| CHECK(simColumns.count("save_avg_stats") == 1); | ||
| CHECK(simColumns.count("save_road_data") == 1); | ||
| CHECK(simColumns.count("save_travel_data") == 1); | ||
| CHECK(simColumns.count("save_agent_data") == 1); |
There was a problem hiding this comment.
Pull request overview
This PR extends the mobility simulation’s SQLite persistence to record per-agent per-edge traversal intervals by introducing a new agent_data table, and updates tests and metadata accordingly.
Changes:
- Add
agent_datatable creation and periodic dumping to SQLite when enabled viaFirstOrderDynamics::saveData(...). - Track edge entry/exit times in
Streetand flush them to the database duringFirstOrderDynamics::evolve(...). - Update dynamics DB tests to validate
agent_data+ addsave_agent_datatosimulationsmetadata; bump DSF patch version and citation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/mobility/Test_dynamics.cpp | Adds coverage for agent_data table creation/content and updates “all options enabled” expectations. |
| src/dsf/mobility/Street.hpp | Introduces static storage + accessors for collecting agent traversal data. |
| src/dsf/mobility/Street.cpp | Records (agent_id, time_in, time_out) when agents leave a street queue. |
| src/dsf/mobility/FirstOrderDynamics.hpp | Extends saveData API to support enabling agent-data persistence. |
| src/dsf/mobility/FirstOrderDynamics.cpp | Creates agent_data, writes rows during save intervals, and adds save_agent_data to simulations. |
| src/dsf/dsf.hpp | Bumps patch version. |
| CITATION.cff | Updates release version/date. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/dsf/mobility/Street.hpp
Outdated
| throw std::runtime_error( | ||
| "agentData not initialized. Please call acquireAgentData() first."); | ||
| } | ||
| return std::move(m_agentData.value()); |
There was a problem hiding this comment.
Street::agentData() moves the underlying map out of the std::optional but leaves m_agentData engaged, so subsequent calls to Street::dequeue() will keep writing into a moved-from tbb::concurrent_unordered_map. This is brittle (moved-from state is unspecified) and makes the lifetime/clearing semantics unclear. Consider extracting via swap/exchange and immediately re-initialize (or reset()) m_agentData so writers always see a well-defined, empty container after a flush.
| return std::move(m_agentData.value()); | |
| decltype(m_agentData.value()) extractedData; | |
| extractedData.swap(m_agentData.value()); | |
| return extractedData; |
| // Create simulations table if it doesn't exist | ||
| SQLite::Statement createTableStmt(*this->database(), | ||
| "CREATE TABLE IF NOT EXISTS simulations (" | ||
| "id INTEGER PRIMARY KEY, " | ||
| "name TEXT, " | ||
| "speed_function TEXT, " | ||
| "weight_function TEXT, " | ||
| "weight_threshold REAL NOT NULL, " | ||
| "error_probability REAL, " | ||
| "passage_probability REAL, " | ||
| "mean_travel_distance_m REAL, " | ||
| "mean_travel_time_s REAL, " | ||
| "stagnant_tolerance_factor REAL, " | ||
| "force_priorities BOOLEAN, " | ||
| "save_avg_stats BOOLEAN, " | ||
| "save_road_data BOOLEAN, " | ||
| "save_travel_data BOOLEAN)"); | ||
| "save_travel_data BOOLEAN, " | ||
| "save_agent_data BOOLEAN)"); | ||
| createTableStmt.exec(); | ||
| // Insert simulation parameters into the simulations table | ||
| SQLite::Statement insertSimStmt( | ||
| *this->database(), | ||
| "INSERT INTO simulations (id, name, speed_function, weight_function, " | ||
| "weight_threshold, error_probability, passage_probability, " | ||
| "mean_travel_distance_m, mean_travel_time_s, stagnant_tolerance_factor, " | ||
| "force_priorities, save_avg_stats, save_road_data, save_travel_data) " | ||
| "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"); | ||
| "force_priorities, save_avg_stats, save_road_data, save_travel_data, " | ||
| "save_agent_data) " | ||
| "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"); | ||
| insertSimStmt.bind(1, static_cast<std::int64_t>(this->id())); |
There was a problem hiding this comment.
CREATE TABLE IF NOT EXISTS simulations (...) will not add the new save_agent_data column when opening an existing DB created with an older schema; the subsequent INSERT INTO simulations (..., save_agent_data) will fail with "no such column". Add a lightweight migration step (e.g., PRAGMA table_info(simulations) + ALTER TABLE simulations ADD COLUMN save_agent_data BOOLEAN DEFAULT 0) before the insert to keep backward compatibility.
| if (m_bSaveAgentData) { | ||
| auto agentData = Street::agentData(); | ||
| SQLite::Statement insertStmt(*this->database(), | ||
| "INSERT INTO agent_data (simulation_id, " | ||
| "agent_id, edge_id, time_step_in, time_step_out)" | ||
| "VALUES (?, ?, ?, ?, ?)"); | ||
| for (auto const& [edge_id, data] : agentData) { | ||
| for (auto const& [agent_id, ts_in, ts_out] : data) { | ||
| insertStmt.bind(1, simulationId); | ||
| insertStmt.bind(2, static_cast<std::int64_t>(agent_id)); | ||
| insertStmt.bind(3, static_cast<std::int64_t>(edge_id)); | ||
| insertStmt.bind(4, static_cast<std::int64_t>(ts_in)); | ||
| insertStmt.bind(5, static_cast<std::int64_t>(ts_out)); | ||
| insertStmt.exec(); | ||
| insertStmt.reset(); | ||
| } |
There was a problem hiding this comment.
Agent-data inserts are not covered by the hasWritePayload/transaction logic (so when only m_bSaveAgentData is enabled, inserts run in autocommit mode). This can drastically slow down writes and makes this block behave differently from the other tables. Update the transaction condition to include agent-data payload and ensure these inserts run inside the same transaction.
| if (m_bSaveAgentData) { | |
| auto agentData = Street::agentData(); | |
| SQLite::Statement insertStmt(*this->database(), | |
| "INSERT INTO agent_data (simulation_id, " | |
| "agent_id, edge_id, time_step_in, time_step_out)" | |
| "VALUES (?, ?, ?, ?, ?)"); | |
| for (auto const& [edge_id, data] : agentData) { | |
| for (auto const& [agent_id, ts_in, ts_out] : data) { | |
| insertStmt.bind(1, simulationId); | |
| insertStmt.bind(2, static_cast<std::int64_t>(agent_id)); | |
| insertStmt.bind(3, static_cast<std::int64_t>(edge_id)); | |
| insertStmt.bind(4, static_cast<std::int64_t>(ts_in)); | |
| insertStmt.bind(5, static_cast<std::int64_t>(ts_out)); | |
| insertStmt.exec(); | |
| insertStmt.reset(); | |
| } | |
| auto agentData = Street::agentData(); | |
| if (m_bSaveAgentData && !agentData.empty()) { | |
| this->database()->exec("SAVEPOINT agent_data_insert"); | |
| try { | |
| SQLite::Statement insertStmt(*this->database(), | |
| "INSERT INTO agent_data (simulation_id, " | |
| "agent_id, edge_id, time_step_in, time_step_out)" | |
| "VALUES (?, ?, ?, ?, ?)"); | |
| for (auto const& [edge_id, data] : agentData) { | |
| for (auto const& [agent_id, ts_in, ts_out] : data) { | |
| insertStmt.bind(1, simulationId); | |
| insertStmt.bind(2, static_cast<std::int64_t>(agent_id)); | |
| insertStmt.bind(3, static_cast<std::int64_t>(edge_id)); | |
| insertStmt.bind(4, static_cast<std::int64_t>(ts_in)); | |
| insertStmt.bind(5, static_cast<std::int64_t>(ts_out)); | |
| insertStmt.exec(); | |
| insertStmt.reset(); | |
| } | |
| } | |
| this->database()->exec("RELEASE SAVEPOINT agent_data_insert"); | |
| } catch (...) { | |
| this->database()->exec("ROLLBACK TO SAVEPOINT agent_data_insert"); | |
| this->database()->exec("RELEASE SAVEPOINT agent_data_insert"); | |
| throw; |
| for (auto const& [agent_id, ts_in, ts_out] : data) { | ||
| insertStmt.bind(1, simulationId); | ||
| insertStmt.bind(2, static_cast<std::int64_t>(agent_id)); | ||
| insertStmt.bind(3, static_cast<std::int64_t>(edge_id)); | ||
| insertStmt.bind(4, static_cast<std::int64_t>(ts_in)); | ||
| insertStmt.bind(5, static_cast<std::int64_t>(ts_out)); | ||
| insertStmt.exec(); | ||
| insertStmt.reset(); |
There was a problem hiding this comment.
ts_in/ts_out are std::time_t, but they’re bound without an explicit cast. On some platforms time_t is not an int64_t, which can lead to narrowing/overload surprises. Cast ts_in/ts_out to std::int64_t when binding to match the INTEGER schema.
| @@ -1652,6 +1697,7 @@ | |||
| m_bSaveStreetData = false; | |||
| m_bSaveTravelData = false; | |||
| m_bSaveAverageStats = false; | |||
There was a problem hiding this comment.
When saveAgentData is enabled, Street::acquireAgentData() makes Street start collecting agent events based on m_agentData.has_value(). However, in the one-shot savingInterval == 0 reset path only m_bSaveAgentData is cleared; the static Street::m_agentData remains engaged, so streets will keep accumulating agent events even though saving is disabled. Add an explicit teardown/clear (e.g., Street::releaseAgentData()/resetAgentData()), or gate collection on a flag that is reset alongside m_bSaveAgentData.
| m_bSaveAverageStats = false; | |
| m_bSaveAverageStats = false; | |
| Street::releaseAgentData(); |
| spdlog::info( | ||
| "Data saving configured: interval={}s, avg_stats={}, street_data={}, " | ||
| "travel_data={}", | ||
| "travel_data={}, agent_data={}", | ||
| savingInterval, | ||
| saveAverageStats, | ||
| saveStreetData, | ||
| saveTravelData); | ||
| saveTravelData, | ||
| saveAgentData); |
There was a problem hiding this comment.
The log message uses interval={}s, but savingInterval is documented as "in time steps" (not seconds). Consider adjusting the unit in the message to avoid misleading logs (e.g., interval={} steps).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #433 +/- ##
==========================================
+ Coverage 87.27% 87.32% +0.04%
==========================================
Files 52 52
Lines 6461 6516 +55
Branches 717 730 +13
==========================================
+ Hits 5639 5690 +51
- Misses 803 804 +1
- Partials 19 22 +3
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:
|
Add
agent_datatable with columns:Solved issues