feat(algorithms, graphs): sort items by group#190
Merged
BrianLusina merged 3 commits intomainfrom Mar 19, 2026
Merged
Conversation
BrianLusina
added
enhancement
Algorithm
Contributor
📝 WalkthroughWalkthroughAdds a new "Sort Items by Groups" graph algorithm with implementation, documentation, and parameterised unit tests to order items respecting prerequisites and group contiguity. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
algorithms/graphs/sort_items_by_group/__init__.py (2)
10-13: Input mutation: thegrouplist is modified in place.The function mutates the caller's
grouplist by reassigning-1values. This side effect could cause unexpected behaviour for callers who reuse the list after callingsort_items. Consider working on a copy instead.♻️ Suggested fix to avoid mutating input
+ # Create a copy to avoid mutating the input + group = group.copy() + # Assign new group IDs to ungrouped items. -1 means ungrouped for i in range(n): if group[i] == -1:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@algorithms/graphs/sort_items_by_group/__init__.py` around lines 10 - 13, The loop for i in range(n) currently mutates the caller's group list by replacing -1 values (group and m); to avoid this, make and use a local copy (e.g., group_copy = group[:] or list(group)) inside sort_items and perform the for i in range(n) assignments against that copy (and use group_copy anywhere later in the function) so the original group passed by the caller remains unchanged while preserving the same numbering logic with m.
30-33: Optional: Consider deduplicating group-level edges.When multiple items from one group depend on items from another group, duplicate edges are added to
group_graph. While the algorithm still produces correct results (the in-degree counts balance with the duplicate edge traversals), this is inefficient for graphs with many inter-group dependencies.♻️ Optional fix using a set to track added edges
+ # Track added group edges to avoid duplicates + added_group_edges: set[tuple[int, int]] = set() + # Build dependency graphs for current in range(n): for previous in before_items[current]: # Add item-level edge item_graph[previous].append(current) item_indegree[current] += 1 # Add group-level edge if group[previous] != group[current]: - group_graph[group[previous]].append(group[current]) - group_indegree[group[current]] += 1 + edge = (group[previous], group[current]) + if edge not in added_group_edges: + added_group_edges.add(edge) + group_graph[group[previous]].append(group[current]) + group_indegree[group[current]] += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@algorithms/graphs/sort_items_by_group/__init__.py` around lines 30 - 33, Multiple duplicate inter-group edges are being added to group_graph and incrementing group_indegree redundantly; modify the logic around group_graph and group_indegree (the block that checks if group[previous] != group[current]) to deduplicate edges by tracking added group-pair edges (e.g., a set of tuples or a set per source group) and only append to group_graph[group[previous]] and increment group_indegree[group[current]] when the pair (group[previous], group[current]) has not been seen before; keep references to the existing symbols group_graph, group_indegree, group, previous, and current so the check is performed inline before mutating those structures.algorithms/graphs/sort_items_by_group/test_sort_items_by_groups.py (1)
32-33: Consider validating constraints rather than exact equality.The problem states that multiple valid orderings may exist. Using
assertEqualfor exact match means the test will fail if the implementation is refactored to produce a different (but still valid) ordering. For more robust tests, consider validating that:
- All
nitems are present exactly once- Dependency order is respected: for each item
i, all items inbefore_items[i]appear beforei- Group contiguity: items of the same group appear consecutively
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@algorithms/graphs/sort_items_by_group/test_sort_items_by_groups.py` around lines 32 - 33, The test currently asserts exact equality between expected and actual, which is brittle; modify the test in test_sort_items_by_groups.py so that after calling actual = sort_items(n, m, group, before_items) you instead (1) verify all n items appear exactly once (compare sorted(actual) to list(range(n)) or counts), (2) build an index map pos[item]=index and assert for each i that every j in before_items[i] satisfies pos[j] < pos[i], and (3) verify group contiguity by grouping items by group id (use the provided group list) and ensuring that in actual each group’s items occupy a single continuous span (no interleaving). Use the variables n, m, group, before_items, and the function sort_items to locate code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@algorithms/graphs/sort_items_by_group/README.md`:
- Around line 140-152: Update the README complexity section to reflect the
actual Kahn topological-sort implementation rather than subset recursion:
replace the O(n·2^n) time and recursive stack description with Time: O(n + m +
E) where n = number of items, m = number of groups, and E = total edges (sum of
lengths of beforeItems[i]) because each node and edge is processed once in
Kahn's algorithm; replace the space section to Space: O(n + m + E) accounting
for adjacency lists, in-degree arrays, and the queue used during Kahn’s
algorithm (exclude output storage if you treat it as external). Ensure you
reference beforeItems, in-degree arrays, adjacency lists, and the queue in the
updated text so readers can map the complexity to the implementation.
---
Nitpick comments:
In `@algorithms/graphs/sort_items_by_group/__init__.py`:
- Around line 10-13: The loop for i in range(n) currently mutates the caller's
group list by replacing -1 values (group and m); to avoid this, make and use a
local copy (e.g., group_copy = group[:] or list(group)) inside sort_items and
perform the for i in range(n) assignments against that copy (and use group_copy
anywhere later in the function) so the original group passed by the caller
remains unchanged while preserving the same numbering logic with m.
- Around line 30-33: Multiple duplicate inter-group edges are being added to
group_graph and incrementing group_indegree redundantly; modify the logic around
group_graph and group_indegree (the block that checks if group[previous] !=
group[current]) to deduplicate edges by tracking added group-pair edges (e.g., a
set of tuples or a set per source group) and only append to
group_graph[group[previous]] and increment group_indegree[group[current]] when
the pair (group[previous], group[current]) has not been seen before; keep
references to the existing symbols group_graph, group_indegree, group, previous,
and current so the check is performed inline before mutating those structures.
In `@algorithms/graphs/sort_items_by_group/test_sort_items_by_groups.py`:
- Around line 32-33: The test currently asserts exact equality between expected
and actual, which is brittle; modify the test in test_sort_items_by_groups.py so
that after calling actual = sort_items(n, m, group, before_items) you instead
(1) verify all n items appear exactly once (compare sorted(actual) to
list(range(n)) or counts), (2) build an index map pos[item]=index and assert for
each i that every j in before_items[i] satisfies pos[j] < pos[i], and (3) verify
group contiguity by grouping items by group id (use the provided group list) and
ensuring that in actual each group’s items occupy a single continuous span (no
interleaving). Use the variables n, m, group, before_items, and the function
sort_items to locate code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 852b3f77-8b9f-4069-b217-9b079c9dd820
⛔ Files ignored due to path filters (3)
algorithms/graphs/sort_items_by_group/images/examples/sort_items_by_groups_respecting_dependencies_example_1.pngis excluded by!**/*.pngalgorithms/graphs/sort_items_by_group/images/examples/sort_items_by_groups_respecting_dependencies_example_2.pngis excluded by!**/*.pngalgorithms/graphs/sort_items_by_group/images/examples/sort_items_by_groups_respecting_dependencies_example_3.pngis excluded by!**/*.png
📒 Files selected for processing (4)
DIRECTORY.mdalgorithms/graphs/sort_items_by_group/README.mdalgorithms/graphs/sort_items_by_group/__init__.pyalgorithms/graphs/sort_items_by_group/test_sort_items_by_groups.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@algorithms/graphs/sort_items_by_group/README.md`:
- Line 27: Update the constraint sentence in the README.md so it reads
"beforeItems[i] does not contain duplicate elements" instead of "duplicates
elements"; locate the line referencing beforeItems[i] in the constraints section
and correct the adjective "duplicates" to "duplicate".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77cf8108-ad0e-4187-aadb-abeebd6af022
📒 Files selected for processing (1)
algorithms/graphs/sort_items_by_group/README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your change:
Sort items by group
Checklist:
Fixes: #{$ISSUE_NO}.Summary by CodeRabbit
New Features
Documentation
Tests