Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #441 +/- ##
==========================================
- Coverage 87.32% 87.26% -0.07%
==========================================
Files 52 52
Lines 6527 6523 -4
Branches 733 732 -1
==========================================
- Hits 5700 5692 -8
- Misses 805 809 +4
Partials 22 22
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.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adjusts network/cartography utilities to fix lane counting for bidirectional roads and to inline small Network accessors/helpers in the header.
Changes:
- Update lane parsing to halve total lane counts for non-oneway OSM roads when building a directed graph.
- Inline
Networkgetters and Cantor hash helper inNetwork.hpp, and simplify some internal loops/comments. - Shorten Doxygen comments for betweenness centrality APIs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/dsf/cartography/cartography.py | Adjusts lane extraction to account for total-lanes-on-bidirectional OSM tagging in a directed representation. |
| src/dsf/base/Network.hpp | Inlines helper/accessor methods and trims comments in centrality implementations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| oneway_val = data.get("oneway", False) | ||
| is_oneway = ( | ||
| oneway_val is True or oneway_val == "yes" or oneway_val == "True" | ||
| ) | ||
| if not is_oneway: | ||
| n = max(n // 2, 1) # integer division, ensure at least 1 |
There was a problem hiding this comment.
oneway parsing is too narrow and will misclassify common OSM values (e.g., "1", "-1", "true", "YES"), causing incorrect halving of lanes on actually-oneway roads. Consider normalizing string values (trim + lowercase) and treating "yes", "true", "1", and "-1" as oneway (and "no", "false", "0" as not oneway), while keeping boolean handling.
| /// where sigma_st is the number of shortest paths from s to t, | ||
| /// and sigma_st(v) is the number of those paths passing through v. | ||
| /// Results are stored via Node::setBetweennessCentrality. | ||
| /// @brief Compute node betweenness centralities using Brandes' algorithm |
There was a problem hiding this comment.
The detailed Doxygen docs for the Brandes centrality methods (constraints like “weights must be positive”, and the definition/normalization of the computed centrality) were removed. Since these are public template APIs, restoring the key behavioral/contract details would prevent misuse and reduce ambiguity for callers.
| /// where sigma_st is the number of shortest paths from s to t, | ||
| /// and sigma_st(e) is the number of those paths using edge e. | ||
| /// Results are stored via Edge::setBetweennessCentrality. | ||
| /// @brief Compute edge betweenness centralities using Brandes' algorithm |
There was a problem hiding this comment.
The detailed Doxygen docs for the Brandes centrality methods (constraints like “weights must be positive”, and the definition/normalization of the computed centrality) were removed. Since these are public template APIs, restoring the key behavioral/contract details would prevent misuse and reduce ambiguity for callers.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| edge_updates["nlanes"] = max( | ||
| min([int(v) for v in lanes_value]), 1 | ||
| ) # Take max if list, ensure at least 1 lane | ||
| n = max(min([int(v) for v in lanes_value]), 1) |
| n = max(min([int(v) for v in lanes_value]), 1) | ||
| else: | ||
| edge_updates["nlanes"] = max(int(lanes_value), 1) | ||
| n = max(int(lanes_value), 1) |
| oneway_val is True or oneway_val == "yes" or oneway_val == "True" | ||
| ) | ||
| if not is_oneway: | ||
| n = max(n // 2, 1) # integer division, ensure at least 1 |
| edge_updates["nlanes"] = max( | ||
| min([int(v) for v in lanes_value]), 1 | ||
| ) # Take max if list, ensure at least 1 lane | ||
| n = max(min([int(v) for v in lanes_value]), 1) |
| n = max(min([int(v) for v in lanes_value]), 1) | ||
| else: | ||
| edge_updates["nlanes"] = max(int(lanes_value), 1) | ||
| n = max(int(lanes_value), 1) |
| oneway_val is True or oneway_val == "yes" or oneway_val == "True" | ||
| ) | ||
| if not is_oneway: | ||
| n = max(n // 2, 1) # integer division, ensure at least 1 |
| std::unordered_map<Id, double> dist; // distance from source | ||
| std::stack<Id> S; | ||
| std::unordered_map<Id, std::vector<Id>> P; | ||
| std::unordered_map<Id, double> sigma; |
| std::stack<Id> S; | ||
| std::unordered_map<Id, std::vector<Id>> P; | ||
| std::unordered_map<Id, double> sigma; | ||
| std::unordered_map<Id, double> dist; |
| std::unordered_map<Id, std::vector<Id>> P; | ||
| std::unordered_map<Id, double> sigma; | ||
| std::unordered_map<Id, double> dist; | ||
|
|
|
|
||
| if (visited.contains(v)) { | ||
| if (visited.contains(v)) | ||
| continue; |
|
|
||
| if (visited.contains(v)) { | ||
| if (visited.contains(v)) | ||
| continue; |
| Id w = edgeObj.target(); | ||
| if (visited.contains(w)) { | ||
| if (visited.contains(w)) | ||
| continue; |
| Id w = edgeObj.target(); | ||
| if (visited.contains(w)) { | ||
| if (visited.contains(w)) | ||
| continue; |
| std::unordered_map<Id, double> delta; | ||
| for (auto const& [nId, _] : m_nodes) { | ||
| for (auto const& [nId, _] : m_nodes) | ||
| delta[nId] = 0.0; |
|
|
||
| if (visited.contains(v)) { | ||
| if (visited.contains(v)) | ||
| continue; |
| Id w = edgeObj.target(); | ||
| if (visited.contains(w)) { | ||
| if (visited.contains(w)) | ||
| continue; |
| Id w = edgeObj.target(); | ||
| if (visited.contains(w)) { | ||
| if (visited.contains(w)) | ||
| continue; |
| std::unordered_map<Id, double> delta; | ||
| for (auto const& [nId, _] : m_nodes) { | ||
| for (auto const& [nId, _] : m_nodes) | ||
| delta[nId] = 0.0; |
| std::unordered_map<Id, double> delta; | ||
| for (auto const& [nId, _] : m_nodes) { | ||
| for (auto const& [nId, _] : m_nodes) | ||
| delta[nId] = 0.0; |
No description provided.