Modernize graph_ops.hpp #711
Conversation
Signed-off-by: Elazar Gershuni <[email protected]>
WalkthroughThe changes involve modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/crab_utils/graph_ops.hpp (31 hunks)
🔇 Additional comments (49)
src/crab_utils/graph_ops.hpp (49)
25-25: Excellent use of a constant for invalid vertex identifiers!Introducing the
invalid_vertconstant to replace the use of-1for invalid vertex identifiers enhances code clarity and reduces potential errors. Great improvement!
32-35: LGTM!The check against
invalid_vertis now used consistently for invalid vertices.
87-91: LGTM!The iterator increment and decrement operators are implemented correctly.
99-102: LGTM!The
vert_const_rangeconstructor and iterator methods are implemented correctly.
126-126: LGTM!The use of
invalid_vertfor skipping invalid vertices in the iterator comparison is correct.
152-152: LGTM!The use of
invalid_vertfor skipping invalid vertices in the edge iterator comparison is correct.
179-185: LGTM!The changes to use
invalid_vertandItG::empty_iterator()in theadj_listclass are correct.
190-190: LGTM!The use of
invalid_vertfor checking invalid vertices in thememmethod is correct.
219-225: LGTM!The changes to use
invalid_vertandItG::empty_iterator()in theconst_adj_listclass are correct.
230-230: LGTM!The use of
invalid_vertfor checking invalid vertices in thememmethod is correct.
248-251: LGTM!The use of
invalid_vertfor checking invalid vertices in thesuccsmethod is correct.
254-257: LGTM!The use of
invalid_vertfor checking invalid vertices in thepredsmethod is correct.
261-264: LGTM!The use of
invalid_vertfor checking invalid vertices in thee_succsmethod is correct.
267-270: LGTM!The use of
invalid_vertfor checking invalid vertices in thee_predsmethod is correct.
292-292: LGTM!The
elemmethod implementation is correct.
294-294: LGTM!The
lookupmethod implementation is correct.
Line range hint
554-564: LGTM!The
grow_scratchfunction implementation looks good. The exponential growth strategy and initialization of new elements are handled correctly.
595-595: LGTM!The use of
std::maxto compute the maximum edge weight during the join operation is correct.
650-653: LGTM!The
strong_connectfunction implementation looks good. The logic for computing strongly connected components is correct.
669-669: LGTM!The use of
gsl::narrowfor narrowing conversion is appropriate here.
674-677: LGTM!The logic for generating strongly connected components when a root node is found is correct.
681-681: LGTM!The use of bitwise AND to clear the visited flag is correct.
687-688: LGTM!The
compute_sccsfunction implementation looks good. It correctly initializes the scratch space and callsstrong_connectfor each unvisited vertex.
696-697: LGTM!The initialization of the stack and index variables is correct.
709-709: LGTM!The
select_potentialsfunction implementation looks good. It correctly computes strongly connected components and runs the Bellman-Ford algorithm on each SCC.
730-730: LGTM!The loop over strongly connected components in reverse topological order is correct.
741-741: LGTM!The increment of the queue tail pointer is correct.
744-745: LGTM!The loop over vertices in the current strongly connected component is correct.
760-760: LGTM!The increment of the next queue tail pointer is correct.
774-775: LGTM!The loop for checking the feasibility of the current strongly connected component is correct.
794-794: LGTM!The
close_after_meetfunction implementation looks good. It correctly partitions edges and runs the chromatic Dijkstra variant on each source vertex.
919-924: LGTM!The
dijkstra_recoverfunction implementation looks good. It correctly runs Dijkstra's algorithm while avoiding the expansion of stable vertices.
993-993: LGTM!The
repair_potentialfunction implementation looks good. It correctly initializes the distances and runs Dijkstra's algorithm to repair the potential.
1045-1045: LGTM!The
close_after_widenfunction implementation looks good. It correctly marks stable vertices and callsdijkstra_recoverfor unstable vertices.
1069-1072: LGTM!The
AdjCmpstruct implementation looks good. It correctly compares vertices based on their slack values.
1077-1077: LGTM!The
NegPstruct implementation looks good. It correctly returns the negated potential value for a given vertex.
1083-1085: LGTM!The
close_after_assign_fwdfunction implementation looks good. It correctly computes the transitive closure of edges reachable from a given vertex.
1102-1103: LGTM!The increment of the adjacent tail pointer is correct.
1106-1106: LGTM!The sorting of immediate edges by increasing slack using
AdjCmpis correct.
1109-1109: LGTM!The loop over adjacent vertices is correct.
1120-1120: LGTM!The increment of the reachable tail pointer is correct.
1129-1129: LGTM!The loop for collecting adjacencies and clearing vertex flags is correct.
1198-1199: LGTM!The
close_after_assignfunction implementation looks good. It correctly callsclose_after_assign_fwdfor both forward and reverse directions.
1211-1212: LGTM!The use of
GraphRevto compute the closure in the reverse direction is correct.
109-109: 🧹 Nitpick (assertive)Use
vert_idfor consistency and consider renaming the parameter.For consistency with the rest of the codebase, use
vert_idinstead ofvert_idfor theafterparameter. Additionally, consider renamingafterto a more descriptive name, such aslast_vert, to clarify its purpose.- vert_const_range verts() const { return vert_const_range(gsl::narrow<vert_id>(perm.size())); } + vert_const_range verts() const { return vert_const_range(gsl::narrow<vert_id>(perm.size())); }Likely invalid or redundant comment.
623-623: 🧹 Nitpick (assertive)Consider using
const auto&for function parameters.To avoid unnecessary copying and maintain const-correctness, consider using
const auto&for thelandrparameters in thewidenfunction.- static graph_t widen(const auto& l, const auto& r, std::unordered_set<vert_id>& unstable) { + static graph_t widen(const auto& l, const auto& r, std::unordered_set<vert_id>& unstable) {Likely invalid or redundant comment.
603-603: 🧹 Nitpick (assertive)Consider using
const auto&for function parameters.To avoid unnecessary copying and maintain const-correctness, consider using
const auto&for thelandrparameters in themeetfunction.- static graph_t meet(const auto& l, const auto& r, bool& is_closed) { + static graph_t meet(const auto& l, const auto& r, bool& is_closed) {Likely invalid or redundant comment.
84-85: 🧹 Nitpick (assertive)Consider using
vert_idtype for consistency.For consistency with the rest of the codebase, consider using the
vert_idtype instead ofvert_idfor the constructor parameter and theoperator*return type.- explicit iterator(const vert_id _v) : v{_v} {} + explicit iterator(vert_id _v) : v{_v} {} - vert_id operator*() const { return v; } + vert_id operator*() const { return v; }Likely invalid or redundant comment.
30-30: Verify that all uses of-1for invalid vertices have been replaced.The constructor now initializes the
invvector withinvalid_vertinstead of-1, which is a positive change. However, please ensure that all other occurrences of-1used for invalid vertices throughout the codebase have been consistently replaced withinvalid_vert.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores