Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a set of reusable graph algorithm implementations (paths, MST, and coloring) along with comprehensive pytest suites to validate their behavior against the existing evaluation_function.schemas models.
Changes:
- Adds a shared
utilsmodule withUnionFind, adjacency builders, degree helpers, connectivity checks, and edge-weight utilities, and wires them into a newevaluation_function.algorithmspackage API. - Implements Eulerian/Hamiltonian path and circuit algorithms, MST algorithms (Kruskal/Prim, verification, forest and visualization helpers), and graph/edge coloring algorithms (greedy, DSatur, chromatic number/index) using the
Graph/Edgeschemas. - Introduces extensive unit tests for path, MST, and coloring behavior, including helper functions, high-level evaluation APIs, feedback generation, edge cases, and simple performance checks.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| evaluation_function/algorithms/utils.py | Introduces shared graph utilities (UnionFind, adjacency list/multiset builders, degree and connectivity helpers, edge-weight lookup) used across algorithm modules. |
| evaluation_function/algorithms/path.py | Implements Eulerian and Hamiltonian path/circuit existence checks, path construction, verification, high-level evaluation APIs, and textual feedback helpers. |
| evaluation_function/algorithms/mst.py | Provides MST algorithms (Kruskal, Prim), auto-selection, verification helpers, forest computation for disconnected graphs, visualization/animation data, and a high-level find_mst / evaluate_mst_submission API. |
| evaluation_function/algorithms/coloring.py | Adds vertex and edge coloring verification, greedy and DSatur vertex coloring, greedy edge coloring, chromatic number/index computation, and high-level color_graph / color_edges helpers. |
| evaluation_function/algorithms/init.py | Exposes a consolidated public API for the new utilities, path, MST, and coloring algorithms via convenient re-exports. |
| tests/path_test.py | Adds extensive tests for Eulerian/Hamiltonian helpers, existence checks, path finders, verifiers, high-level APIs, and feedback generation, plus directed/multigraph edge cases. |
| tests/mst_test.py | Adds comprehensive tests for UnionFind, helper functions, Kruskal/Prim MST implementations, auto-selection, verification, disconnected-graph handling, visualization/animation hooks, and integration workflows. |
| tests/coloring_test.py | Adds thorough tests for vertex and edge coloring verification, greedy and DSatur algorithms, chromatic number/index computation, high-level coloring APIs, and various edge-case/algorithm-comparison scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif algorithm == "backtracking": | ||
| # For backtracking, we find the chromatic number and color with that | ||
| chi = compute_chromatic_number(graph) | ||
| if chi is None: | ||
| # Fall back to DSatur for large graphs | ||
| coloring = dsatur_coloring(graph) | ||
| else: | ||
| coloring = dsatur_coloring(graph) | ||
| else: |
There was a problem hiding this comment.
The "backtracking" branch of color_graph computes the chromatic number via compute_chromatic_number but then ignores it and always falls back to dsatur_coloring, which contradicts the docstring that says backtracking should "find the chromatic number and color with that". This makes the algorithm="backtracking" mode misleading and potentially non-optimal; either use the backtracking result to drive the coloring (e.g., actually color with k=chi) or update the documentation and parameter description to reflect the current behavior.
| build_edge_set, | ||
| get_edge_weight, | ||
| is_connected, | ||
| count_components, |
There was a problem hiding this comment.
Import of 'count_components' is not used.
| count_components, |
| """ | ||
|
|
||
| from typing import Optional | ||
| from collections import defaultdict, deque |
There was a problem hiding this comment.
Import of 'deque' is not used.
| from collections import defaultdict, deque | |
| from collections import defaultdict |
| from .utils import ( | ||
| build_adjacency_list, | ||
| build_adjacency_multiset, | ||
| get_in_out_degree, | ||
| is_connected, | ||
| is_weakly_connected, | ||
| ) |
There was a problem hiding this comment.
Import of 'build_adjacency_multiset' is not used.
| from evaluation_function.algorithms.path import ( | ||
| # Helper functions | ||
| build_adjacency_list, | ||
| build_adjacency_multiset, | ||
| get_degree, | ||
| get_in_out_degree, | ||
| is_connected_undirected, | ||
| is_weakly_connected_directed, | ||
|
|
||
| # Eulerian existence checks | ||
| check_eulerian_undirected, | ||
| check_eulerian_directed, | ||
| check_eulerian_existence, | ||
|
|
||
| # Eulerian path finding | ||
| find_eulerian_path_undirected, | ||
| find_eulerian_path_directed, | ||
| find_eulerian_path, | ||
| find_eulerian_circuit, | ||
|
|
||
| # Eulerian verification | ||
| verify_eulerian_path, | ||
|
|
||
| # Hamiltonian verification | ||
| verify_hamiltonian_path, | ||
|
|
||
| # Hamiltonian existence | ||
| find_hamiltonian_path_backtrack, | ||
| check_hamiltonian_existence, | ||
|
|
||
| # High-level API | ||
| evaluate_eulerian_path, | ||
| evaluate_hamiltonian_path, | ||
| get_eulerian_feedback, | ||
| get_hamiltonian_feedback, | ||
| ) |
There was a problem hiding this comment.
Import of 'build_adjacency_multiset' is not used.
Import of 'get_degree' is not used.
Import of 'find_hamiltonian_path_backtrack' is not used.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| has_path, has_circuit, odd_vertices, reason = check_eulerian_undirected(graph) | ||
| adj = build_adjacency_list(graph) | ||
|
|
||
| if not is_connected_undirected(graph): | ||
| feedback.append("The graph is not connected. An Eulerian path/circuit requires all edges to be in a single connected component.") | ||
| return feedback | ||
|
|
||
| if check_circuit: | ||
| if has_circuit: | ||
| feedback.append("An Eulerian circuit exists! All vertices have even degree.") | ||
| else: | ||
| feedback.append("No Eulerian circuit exists.") | ||
| for v in odd_vertices: | ||
| feedback.append(f" - Vertex {v} has odd degree ({len(adj.get(v, set()))})") | ||
| feedback.append("For a circuit, all vertices must have even degree.") | ||
| else: |
There was a problem hiding this comment.
In the undirected branch of get_eulerian_feedback, the degree reported for odd-degree vertices is computed via len(adj.get(v, set())), which ignores edge multiplicity and self-loops, whereas check_eulerian_undirected above counts self-loops as 2 and accounts for multiedges when determining odd-degree vertices. On multigraphs like the konigsberg_bridge fixture, this will produce incorrect numeric degrees in the feedback (e.g., vertex A will be reported with degree 3 instead of 5), even though the odd/even classification is correct. To keep feedback accurate and consistent with the existence check, consider reusing the degree map from check_eulerian_undirected (e.g., by returning it or recomputing with the same logic) instead of deriving degrees from the adjacency set size.
| k: Number of colors to use (optional, only for backtracking) | ||
|
|
||
| Returns: | ||
| ColoringResult with the coloring and details |
There was a problem hiding this comment.
The color_graph docstring describes k as "Number of colors to use (optional, only for backtracking)", but the implementation never passes k into the backtracking search and instead uses it only as a post-hoc constraint in verify_vertex_coloring, applied uniformly to all algorithms. This mismatch can be confusing to callers who might expect the backtracking branch to attempt a coloring with exactly k colors. Either wire k into the backtracking search (e.g., limit the color range in the search to k and/or fail fast when k is infeasible) or update the docstring to clarify that k is treated purely as a validation constraint rather than a search limit.
| k: Number of colors to use (optional, only for backtracking) | |
| Returns: | |
| ColoringResult with the coloring and details | |
| k: Optional upper bound on the number of colors; used to validate | |
| the resulting coloring (applies to all algorithms, does not | |
| constrain the search itself). | |
| Returns: | |
| ColoringResult with the coloring and validation details |
| # Binary search for chromatic number | ||
| # Lower bound: 1 (if graph is empty) or 2 (if has edges) or clique size | ||
| # Upper bound: n or max_degree + 1 | ||
|
|
||
| # Calculate max degree | ||
| max_degree = max((len(neighbors) for neighbors in adj.values()), default=0) | ||
|
|
There was a problem hiding this comment.
Variable max_degree is not used.
| # Binary search for chromatic number | |
| # Lower bound: 1 (if graph is empty) or 2 (if has edges) or clique size | |
| # Upper bound: n or max_degree + 1 | |
| # Calculate max degree | |
| max_degree = max((len(neighbors) for neighbors in adj.values()), default=0) | |
| # Search for chromatic number | |
| # Lower bound: 1 (if graph is empty) or 2 (if has edges) or clique size | |
| # Upper bound: n | |
| num_colors = len(set(coloring.values())) if coloring else 0 | ||
|
|
There was a problem hiding this comment.
Variable num_colors is not used.
| num_colors = len(set(coloring.values())) if coloring else 0 | |
| is_valid, error = verify_eulerian_path(graph, submitted_path, check_circuit) | ||
|
|
||
| # Also check start/end constraints | ||
| if is_valid and start_node and submitted_path and submitted_path[0] != start_node: | ||
| is_valid = False | ||
| error = f"Path must start at {start_node}, but starts at {submitted_path[0]}" | ||
| if is_valid and end_node and submitted_path and submitted_path[-1] != end_node: | ||
| is_valid = False | ||
| error = f"Path must end at {end_node}, but ends at {submitted_path[-1]}" |
There was a problem hiding this comment.
Variable error is not used.
| is_valid, error = verify_eulerian_path(graph, submitted_path, check_circuit) | |
| # Also check start/end constraints | |
| if is_valid and start_node and submitted_path and submitted_path[0] != start_node: | |
| is_valid = False | |
| error = f"Path must start at {start_node}, but starts at {submitted_path[0]}" | |
| if is_valid and end_node and submitted_path and submitted_path[-1] != end_node: | |
| is_valid = False | |
| error = f"Path must end at {end_node}, but ends at {submitted_path[-1]}" | |
| is_valid, _ = verify_eulerian_path(graph, submitted_path, check_circuit) | |
| # Also check start/end constraints | |
| if is_valid and start_node and submitted_path and submitted_path[0] != start_node: | |
| is_valid = False | |
| if is_valid and end_node and submitted_path and submitted_path[-1] != end_node: | |
| is_valid = False |
| # Also check start/end constraints | ||
| if is_valid and start_node and submitted_path and submitted_path[0] != start_node: | ||
| is_valid = False | ||
| error = f"Path must start at {start_node}, but starts at {submitted_path[0]}" |
| is_valid, error = verify_hamiltonian_path(graph, submitted_path, check_circuit) | ||
|
|
||
| # Check start/end constraints | ||
| if is_valid and start_node and submitted_path and submitted_path[0] != start_node: | ||
| is_valid = False | ||
| error = f"Path must start at {start_node}" | ||
| if is_valid and end_node and submitted_path: | ||
| end_check = submitted_path[-1] if not check_circuit else submitted_path[-2] if len(submitted_path) > 1 else submitted_path[0] | ||
| if end_check != end_node: | ||
| is_valid = False | ||
| error = f"Path must end at {end_node}" |
There was a problem hiding this comment.
This assignment to 'error' is unnecessary as it is redefined before this value is used.
| is_valid, error = verify_hamiltonian_path(graph, submitted_path, check_circuit) | |
| # Check start/end constraints | |
| if is_valid and start_node and submitted_path and submitted_path[0] != start_node: | |
| is_valid = False | |
| error = f"Path must start at {start_node}" | |
| if is_valid and end_node and submitted_path: | |
| end_check = submitted_path[-1] if not check_circuit else submitted_path[-2] if len(submitted_path) > 1 else submitted_path[0] | |
| if end_check != end_node: | |
| is_valid = False | |
| error = f"Path must end at {end_node}" | |
| is_valid, _ = verify_hamiltonian_path(graph, submitted_path, check_circuit) | |
| # Check start/end constraints | |
| if is_valid and start_node and submitted_path and submitted_path[0] != start_node: | |
| is_valid = False | |
| if is_valid and end_node and submitted_path: | |
| end_check = submitted_path[-1] if not check_circuit else submitted_path[-2] if len(submitted_path) > 1 else submitted_path[0] | |
| if end_check != end_node: | |
| is_valid = False |
| from typing import Optional | ||
| from collections import defaultdict | ||
|
|
||
| from ..schemas.graph import Graph, Node, Edge |
There was a problem hiding this comment.
Import of 'Node' is not used.
Import of 'Edge' is not used.
| from ..schemas.graph import Graph, Node, Edge | |
| from ..schemas.graph import Graph |
| from collections import defaultdict, deque | ||
| import time | ||
|
|
||
| from ..schemas.graph import Graph, Node, Edge |
There was a problem hiding this comment.
Import of 'Node' is not used.
Import of 'Edge' is not used.
| from ..schemas.graph import Graph, Node, Edge | |
| from ..schemas.graph import Graph |
| from .utils import ( | ||
| build_adjacency_list, | ||
| build_adjacency_multiset, | ||
| get_in_out_degree, | ||
| is_connected, | ||
| is_weakly_connected, | ||
| ) |
There was a problem hiding this comment.
Import of 'build_adjacency_multiset' is not used.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
|
|
||
| from typing import Optional, Literal | ||
| from collections import defaultdict |
There was a problem hiding this comment.
defaultdict is imported but never used in this module; removing the unused import will keep the module clean and avoid linter warnings.
| from collections import defaultdict |
| Args: | ||
| graph: The Graph object | ||
| algorithm: "greedy", "dsatur", "backtracking", or "auto" | ||
| k: Number of colors to use (optional, only for backtracking) |
There was a problem hiding this comment.
The color_graph docstring says k is "only for backtracking", but the implementation always forwards k to verify_vertex_coloring, so the color limit is enforced for greedy and DSatur runs as well (as used in the tests). To avoid confusing users, please update the parameter description to reflect the actual behavior or change the implementation if you intended k to apply only to the backtracking variant.
| k: Number of colors to use (optional, only for backtracking) | |
| k: Optional maximum number of colors to allow in the coloring | |
| (enforced during verification for all algorithms). |
| # Upper bound: n or max_degree + 1 | ||
|
|
||
| # Calculate max degree | ||
| max_degree = max((len(neighbors) for neighbors in adj.values()), default=0) |
There was a problem hiding this comment.
Variable max_degree is not used.
| is_valid, error = verify_eulerian_path(graph, submitted_path, check_circuit) | ||
|
|
||
| # Also check start/end constraints | ||
| if is_valid and start_node and submitted_path and submitted_path[0] != start_node: | ||
| is_valid = False | ||
| error = f"Path must start at {start_node}, but starts at {submitted_path[0]}" | ||
| if is_valid and end_node and submitted_path and submitted_path[-1] != end_node: | ||
| is_valid = False | ||
| error = f"Path must end at {end_node}, but ends at {submitted_path[-1]}" |
There was a problem hiding this comment.
This assignment to 'error' is unnecessary as it is redefined before this value is used.
| is_valid, error = verify_eulerian_path(graph, submitted_path, check_circuit) | |
| # Also check start/end constraints | |
| if is_valid and start_node and submitted_path and submitted_path[0] != start_node: | |
| is_valid = False | |
| error = f"Path must start at {start_node}, but starts at {submitted_path[0]}" | |
| if is_valid and end_node and submitted_path and submitted_path[-1] != end_node: | |
| is_valid = False | |
| error = f"Path must end at {end_node}, but ends at {submitted_path[-1]}" | |
| is_valid, _ = verify_eulerian_path(graph, submitted_path, check_circuit) | |
| # Also check start/end constraints | |
| if is_valid and start_node and submitted_path and submitted_path[0] != start_node: | |
| is_valid = False | |
| if is_valid and end_node and submitted_path and submitted_path[-1] != end_node: | |
| is_valid = False |
1 participant
No description provided.