diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c2db3d6..b6591c9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,10 +15,10 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - name: Install uv - uses: astral-sh/setup-uv@v7 + uses: astral-sh/setup-uv@v9.0.0 with: enable-cache: true cache-dependency-glob: "uv.lock" @@ -57,10 +57,10 @@ jobs: matrix: python-version: ["3.11", "3.12", "3.13"] steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - name: Install uv - uses: astral-sh/setup-uv@v7 + uses: astral-sh/setup-uv@v9.0.0 with: enable-cache: true cache-dependency-glob: "uv.lock" @@ -77,7 +77,7 @@ jobs: uv run pytest tests/ --cov=allocator --cov-report=xml --cov-report=term-missing -v --tb=short - name: Upload coverage to Codecov - uses: codecov/codecov-action@v5 + uses: codecov/codecov-action@v7 if: matrix.python-version == '3.11' with: file: ./coverage.xml @@ -87,10 +87,10 @@ jobs: test-algorithms: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - name: Install uv - uses: astral-sh/setup-uv@v7 + uses: astral-sh/setup-uv@v9.0.0 with: enable-cache: true cache-dependency-glob: "uv.lock" @@ -104,7 +104,7 @@ jobs: - name: Test advanced algorithms run: | - uv run pytest tests/api/ -k "ortools or google" -v --tb=short + uv run pytest tests/api/ -k "ortools or google or christofides" -v --tb=short # Every other job installs from uv.lock, so CI and a developer's machine agree. # That is the point, and it has a cost: CI stops noticing when a new release @@ -120,19 +120,17 @@ jobs: runs-on: ubuntu-latest if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - name: Install uv - uses: astral-sh/setup-uv@v7 + uses: astral-sh/setup-uv@v9.0.0 - name: Set up Python 3.11 run: uv python install 3.11 - # The same extras the lint and test jobs use, deliberately not --all-extras: - # tests/api/test_route_api.py::test_christofides_import_error_handling - # asserts that tsp_christofides *raises* ImportError, so installing the - # algorithms extra here would fail it for reasons that have nothing to do - # with upstream drift. + # The same extras the lint and test jobs use, deliberately not --all-extras, + # so this job checks the install most people actually have rather than the + # maximal one. - name: Resolve the latest of everything, ignoring the lock run: uv sync --upgrade --extra dev --extra test --all-groups @@ -152,10 +150,10 @@ jobs: runs-on: ubuntu-latest needs: [lint, test] steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - name: Install uv - uses: astral-sh/setup-uv@v7 + uses: astral-sh/setup-uv@v9.0.0 - name: Set up Python 3.11 run: | diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index 2263cbd..7388fe5 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -19,10 +19,10 @@ jobs: build: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - name: Install uv - uses: astral-sh/setup-uv@v7 + uses: astral-sh/setup-uv@v9.0.0 with: enable-cache: true cache-dependency-glob: "uv.lock" @@ -49,12 +49,17 @@ jobs: touch build/html/.nojekyll - name: Setup Pages - uses: actions/configure-pages@v5 + uses: actions/configure-pages@v6 - name: Upload artifact - uses: actions/upload-pages-artifact@v3 + uses: actions/upload-pages-artifact@v5 with: path: 'docs/build/html' + # v4 started excluding dotfiles from the artifact. The build step writes + # .nojekyll, and Sphinx output is full of _static and _sources, so losing + # it is the kind of change that shows up as a broken site rather than a + # failed job. + include-hidden-files: true deploy: environment: @@ -66,4 +71,4 @@ jobs: steps: - name: Deploy to GitHub Pages id: deployment - uses: actions/deploy-pages@v4 \ No newline at end of file + uses: actions/deploy-pages@v5 \ No newline at end of file diff --git a/.github/workflows/python-publish.yml b/.github/workflows/python-publish.yml index 80e663f..26de7ce 100644 --- a/.github/workflows/python-publish.yml +++ b/.github/workflows/python-publish.yml @@ -21,10 +21,10 @@ jobs: test-before-publish: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 - name: Install uv - uses: astral-sh/setup-uv@v7 + uses: astral-sh/setup-uv@v9.0.0 with: enable-cache: true cache-dependency-glob: "uv.lock" @@ -48,12 +48,12 @@ jobs: url: ${{ github.event.inputs.use_test_pypi == 'true' && 'https://test.pypi.org/p/allocator' || 'https://pypi.org/p/allocator' }} steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v7 with: fetch-depth: 0 - name: Install uv - uses: astral-sh/setup-uv@v7 + uses: astral-sh/setup-uv@v9.0.0 - name: Set up Python 3.11 run: | diff --git a/allocator/core/routing.py b/allocator/core/routing.py index fd14db0..6934b82 100644 --- a/allocator/core/routing.py +++ b/allocator/core/routing.py @@ -99,13 +99,15 @@ def solve_tsp_christofides( Returns: (total_distance, route) tuple """ - try: - from Christofides import christofides - except ImportError as e: - raise ImportError( - "Christofides algorithm requires the 'Christofides' package. " - "Install it with: pip install Christofides" - ) from e + # networkx's own approximation, not the Christofides package on PyPI. That + # package is Python 2 source -- importing it raises SyntaxError on `print + # 'Testing...'` -- so this function had never once run on Python 3. Nothing + # noticed because the only test asserted the package was *absent*, and the + # CI job that installed it filtered the christofides tests out. + # + # networkx is already a hard dependency and has provided this since 2.6, so + # there is no optional import to guard. + from networkx.algorithms.approximation import christofides # Get distance matrix distances = get_distance_matrix(points, points, method=distance_method, **distance_kwargs) @@ -121,15 +123,20 @@ def solve_tsp_christofides( for j in range(i + 1, n): G.add_edge(i, j, weight=distances[i, j]) - # Solve using Christofides algorithm - tour = christofides(G, 0) # Start from node 0 + # A Hamiltonian cycle, so the last node repeats the first. + cycle = christofides(G, weight="weight") - # Calculate total distance + # Length of the closed tour, including the leg back to the start. total_distance = 0.0 - for i in range(len(tour) - 1): - total_distance += distances[tour[i], tour[i + 1]] - - return total_distance, tour + for i in range(len(cycle) - 1): + total_distance += distances[cycle[i], cycle[i + 1]] + + # Return the closed cycle, first node repeated at the end, matching + # solve_tsp_ortools. The two solvers are interchangeable through + # shortest_path(), so a caller switching methods must not silently get a + # different route convention -- and tests/api/test_route_api.py pins the + # ortools one at len(points) + 1. + return float(total_distance), list(cycle) def solve_tsp_osrm( diff --git a/tests/api/test_route_api.py b/tests/api/test_route_api.py index 86d9c02..733dba9 100644 --- a/tests/api/test_route_api.py +++ b/tests/api/test_route_api.py @@ -2,6 +2,7 @@ Tests for the modern routing API. """ +import itertools import unittest import numpy as np @@ -9,6 +10,7 @@ from allocator.api import shortest_path, tsp_christofides, tsp_ortools from allocator.api.types import RouteResult +from allocator.distances import get_distance_matrix class TestRouteAPI(unittest.TestCase): @@ -84,12 +86,70 @@ def test_high_level_shortest_path_function(self): except ImportError: self.skipTest("OR-Tools not available") - def test_christofides_import_error_handling(self): - """Test Christofides handles missing dependencies gracefully.""" - with self.assertRaises(ImportError) as cm: - tsp_christofides(self.test_points) + def test_christofides_solves_a_tour(self): + """Christofides returns a tour that visits every point exactly once. + + This replaces ``test_christofides_import_error_handling``, which asserted + that ``tsp_christofides`` *raises* ImportError. That only happens when the + ``algorithms`` extra is absent, so the test meant one thing in the plain + test job and the opposite in test-algorithms -- and it passed only + because nobody had installed the extra on that runner. + + Running it the other way is what showed the function had never worked on + Python 3: the Christofides package on PyPI is Python 2 source and raises + SyntaxError on import. It now uses networkx's approximation, which is a + hard dependency already, so there is no optional import left to test. + """ + result = tsp_christofides(self.test_points) + n_points = len(self.test_points) + + self.assertIsInstance(result, RouteResult) + # Closed tour, matching tsp_ortools: every point once, then back to the + # start. The two solvers are interchangeable through shortest_path(), so + # they must agree on what a route is. + self.assertEqual(len(result.route), n_points + 1) + self.assertEqual(result.route[0], result.route[-1]) + self.assertEqual(sorted(result.route[:-1]), list(range(n_points))) + self.assertGreater(result.total_distance, 0) + + def test_christofides_stays_within_its_approximation_guarantee(self): + """The property that makes Christofides worth using over any other tour. + + It is a 3/2-approximation on a metric instance, so on a problem small + enough to solve exactly the tour must be no worse than 1.5x optimal. A + merely "valid" tour -- every point once, positive length -- would also be + produced by visiting the points in input order, so without this the test + above does not distinguish the algorithm from doing nothing. + """ + result = tsp_christofides(self.test_points) + + # Score the tour in the metric the solver actually optimised in. + # `euclidean` projects lon/lat to UTM metres before measuring + # (allocator/distances/euclidean.py, utm.from_latlon), so scoring with + # distances computed from raw degrees would compare a UTM-optimal tour + # against a degree-optimal one. That is a different problem, and its + # ratio can exceed 3/2 through projection distortion alone. + points = self.test_points[["longitude", "latitude"]].to_numpy() + n = len(points) + distances = get_distance_matrix(points, points, method="euclidean") + + optimal = min( + sum(distances[order[i], order[i + 1]] for i in range(n - 1)) + + distances[order[-1], order[0]] + for order in itertools.permutations(range(n)) + ) + # result.route is the closed tour; drop the repeated start to iterate. + route = result.route[:-1] + tour = ( + sum(distances[route[i], route[i + 1]] for i in range(n - 1)) + + distances[route[-1], route[0]] + ) - self.assertIn("Christofides", str(cm.exception)) + self.assertLessEqual( + tour, + 1.5 * optimal, + f"tour {tour:.4f} exceeds 1.5x the optimum {optimal:.4f}", + ) def test_invalid_method(self): """Test error handling for invalid TSP method."""