-
Notifications
You must be signed in to change notification settings - Fork 4
Kill random agents exponentially #377
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,7 +34,9 @@ | |||||||||||||||||||||
| std::optional<Id> m_nextStreetId; | ||||||||||||||||||||||
| size_t m_itineraryIdx; | ||||||||||||||||||||||
| double m_speed; | ||||||||||||||||||||||
| double m_distance; // Travelled distance | ||||||||||||||||||||||
| double m_distance; // Travelled distance | ||||||||||||||||||||||
| std::optional<double> m_maxDistance; // Maximum distance for stochastic agents | ||||||||||||||||||||||
| std::optional<std::time_t> m_maxTime; // Maximum time for stochastic agents | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| public: | ||||||||||||||||||||||
| /// @brief Construct a new Agent object | ||||||||||||||||||||||
|
|
@@ -73,6 +75,15 @@ | |||||||||||||||||||||
| /// @brief Update the agent's itinerary | ||||||||||||||||||||||
| /// @details If possible, the agent's itinerary is updated by removing the first element | ||||||||||||||||||||||
| /// from the itinerary's vector. | ||||||||||||||||||||||
| inline void setMaxDistance(double const maxDistance) { | ||||||||||||||||||||||
| maxDistance > 0. ? m_maxDistance = maxDistance | ||||||||||||||||||||||
| : throw std::invalid_argument( | ||||||||||||||||||||||
| "Agent::setMaxDistance: maxDistance must be positive"); | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
Comment on lines
+78
to
+82
|
||||||||||||||||||||||
| maxDistance > 0. ? m_maxDistance = maxDistance | |
| : throw std::invalid_argument( | |
| "Agent::setMaxDistance: maxDistance must be positive"); | |
| }; | |
| if (maxDistance > 0.) { | |
| m_maxDistance = maxDistance; | |
| } else { | |
| throw std::invalid_argument("Agent::setMaxDistance: maxDistance must be positive"); | |
| } | |
| } |
Grufoony marked this conversation as resolved.
Show resolved
Hide resolved
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.
[nitpick] The maxDistance() getter method documentation says it returns the agent's maximum distance "or throw std::logic_error if not set", but the actual implementation throws the error inline within the function using a ternary operator. This is unconventional. It would be clearer to use a normal if-else statement for better readability and to follow the pattern of other error-throwing methods in this codebase (see itineraryId() as an example).
| return m_maxDistance.has_value() | |
| ? m_maxDistance.value() | |
| : throw std::logic_error("Agent::maxDistance: maxDistance is not set"); | |
| if (m_maxDistance.has_value()) { | |
| return m_maxDistance.value(); | |
| } else { | |
| throw std::logic_error("Agent::maxDistance: maxDistance is not set"); | |
| } |
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 new maxDistance() getter method lacks test coverage. Consider adding tests to verify:
- Retrieving a valid max distance that was previously set
- Attempting to retrieve max distance when not set throws
std::logic_errorwith the appropriate message
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -63,8 +63,8 @@ namespace dsf::mobility { | |||||||||||||||||||||
| std::function<double(std::unique_ptr<Street> const&)> m_weightFunction; | ||||||||||||||||||||||
| std::optional<double> m_errorProbability; | ||||||||||||||||||||||
| std::optional<double> m_passageProbability; | ||||||||||||||||||||||
| double m_maxTravelDistance; | ||||||||||||||||||||||
| std::time_t m_maxTravelTime; | ||||||||||||||||||||||
| std::optional<double> m_meanTravelDistance; | ||||||||||||||||||||||
| std::optional<std::time_t> m_meanTravelTime; | ||||||||||||||||||||||
| double m_weightTreshold; | ||||||||||||||||||||||
| std::optional<double> m_timeToleranceFactor; | ||||||||||||||||||||||
| std::optional<delay_t> m_dataUpdatePeriod; | ||||||||||||||||||||||
|
|
@@ -151,20 +151,19 @@ namespace dsf::mobility { | |||||||||||||||||||||
| inline void setDataUpdatePeriod(delay_t dataUpdatePeriod) noexcept { | ||||||||||||||||||||||
| m_dataUpdatePeriod = dataUpdatePeriod; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| /// @brief Set the maximum distance which a random agent can travel | ||||||||||||||||||||||
| /// @param maxDistance The maximum distance | ||||||||||||||||||||||
| /// @throw std::invalid_argument If the maximum distance is negative | ||||||||||||||||||||||
| inline void setMaxDistance(double const maxDistance) { | ||||||||||||||||||||||
| if (maxDistance < 0.) { | ||||||||||||||||||||||
| throw std::invalid_argument(std::format( | ||||||||||||||||||||||
| "Maximum travel distance ({}) must be non-negative", maxDistance)); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| m_maxTravelDistance = maxDistance; | ||||||||||||||||||||||
| /// @brief Set the mean distance travelled by a random agent. The distance will be sampled from an exponential distribution with this mean. | ||||||||||||||||||||||
| /// @param meanTravelDistance The mean distance | ||||||||||||||||||||||
| /// @throw std::invalid_argument If the mean distance is negative | ||||||||||||||||||||||
|
||||||||||||||||||||||
| /// @throw std::invalid_argument If the mean distance is negative | |
| /// @throw std::invalid_argument If the mean distance is not positive |
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.
[nitpick] The validation logic uses a ternary operator to throw an exception, which is unconventional and reduces readability. Consider using a traditional if-statement for consistency with other validation methods in the codebase (see setSpeed() in Agent.cpp as an example).
| meanTravelDistance > 0. ? m_meanTravelDistance = meanTravelDistance | |
| : throw std::invalid_argument( | |
| "RoadDynamics::setMeanTravelDistance: " | |
| "meanTravelDistance must be positive"); | |
| if (meanTravelDistance > 0.) { | |
| m_meanTravelDistance = meanTravelDistance; | |
| } else { | |
| throw std::invalid_argument( | |
| "RoadDynamics::setMeanTravelDistance: meanTravelDistance must be positive"); | |
| } |
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 new setMeanTravelDistance() method lacks test coverage. Consider adding tests to verify:
- Setting a valid positive mean travel distance
- Attempting to set a non-positive value throws
std::invalid_argumentwith the appropriate message - The mean distance affects agent creation in
addRandomAgents()
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 exponential distribution is created with the rate parameter 1. / m_meanTravelDistance.value_or(1.). If m_meanTravelDistance is not set, this creates a distribution with mean 1.0, and then the distance is sampled and assigned via setMaxDistance() at lines 1378-1381. However, the condition at line 1378 checks if (m_meanTravelDistance.has_value()), which means when m_meanTravelDistance is not set, no max distance is assigned to agents. This makes the value_or(1.) fallback in line 1359 unnecessary and misleading, since the distribution is only used when m_meanTravelDistance has a value. The distribution initialization should be moved inside the if-block at line 1378.
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.
[nitpick] The variable name distDist (short for "distance distribution") is ambiguous and unclear. Consider a more descriptive name like travelDistanceDist or exponentialTravelDist to better indicate what this distribution represents.
Check warning
Code scanning / Cppcheck (reported by Codacy)
AST broken, ternary operator missing operand(s) Warning