Skip to content

Commit b803848

Browse files
authored
Merge pull request #5 from geosensing/chore/latest-actions-and-deterministic-christofides
Bump actions to current majors, and make tsp_christofides actually work
2 parents f5f7e8a + eda430a commit b803848

5 files changed

Lines changed: 115 additions & 45 deletions

File tree

.github/workflows/ci.yml

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ jobs:
1515
lint:
1616
runs-on: ubuntu-latest
1717
steps:
18-
- uses: actions/checkout@v4
18+
- uses: actions/checkout@v7
1919

2020
- name: Install uv
21-
uses: astral-sh/setup-uv@v7
21+
uses: astral-sh/setup-uv@v9.0.0
2222
with:
2323
enable-cache: true
2424
cache-dependency-glob: "uv.lock"
@@ -57,10 +57,10 @@ jobs:
5757
matrix:
5858
python-version: ["3.11", "3.12", "3.13"]
5959
steps:
60-
- uses: actions/checkout@v4
60+
- uses: actions/checkout@v7
6161

6262
- name: Install uv
63-
uses: astral-sh/setup-uv@v7
63+
uses: astral-sh/setup-uv@v9.0.0
6464
with:
6565
enable-cache: true
6666
cache-dependency-glob: "uv.lock"
@@ -77,7 +77,7 @@ jobs:
7777
uv run pytest tests/ --cov=allocator --cov-report=xml --cov-report=term-missing -v --tb=short
7878
7979
- name: Upload coverage to Codecov
80-
uses: codecov/codecov-action@v5
80+
uses: codecov/codecov-action@v7
8181
if: matrix.python-version == '3.11'
8282
with:
8383
file: ./coverage.xml
@@ -87,10 +87,10 @@ jobs:
8787
test-algorithms:
8888
runs-on: ubuntu-latest
8989
steps:
90-
- uses: actions/checkout@v4
90+
- uses: actions/checkout@v7
9191

9292
- name: Install uv
93-
uses: astral-sh/setup-uv@v7
93+
uses: astral-sh/setup-uv@v9.0.0
9494
with:
9595
enable-cache: true
9696
cache-dependency-glob: "uv.lock"
@@ -104,7 +104,7 @@ jobs:
104104

105105
- name: Test advanced algorithms
106106
run: |
107-
uv run pytest tests/api/ -k "ortools or google" -v --tb=short
107+
uv run pytest tests/api/ -k "ortools or google or christofides" -v --tb=short
108108
109109
# Every other job installs from uv.lock, so CI and a developer's machine agree.
110110
# That is the point, and it has a cost: CI stops noticing when a new release
@@ -120,19 +120,17 @@ jobs:
120120
runs-on: ubuntu-latest
121121
if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch'
122122
steps:
123-
- uses: actions/checkout@v4
123+
- uses: actions/checkout@v7
124124

125125
- name: Install uv
126-
uses: astral-sh/setup-uv@v7
126+
uses: astral-sh/setup-uv@v9.0.0
127127

128128
- name: Set up Python 3.11
129129
run: uv python install 3.11
130130

131-
# The same extras the lint and test jobs use, deliberately not --all-extras:
132-
# tests/api/test_route_api.py::test_christofides_import_error_handling
133-
# asserts that tsp_christofides *raises* ImportError, so installing the
134-
# algorithms extra here would fail it for reasons that have nothing to do
135-
# with upstream drift.
131+
# The same extras the lint and test jobs use, deliberately not --all-extras,
132+
# so this job checks the install most people actually have rather than the
133+
# maximal one.
136134
- name: Resolve the latest of everything, ignoring the lock
137135
run: uv sync --upgrade --extra dev --extra test --all-groups
138136

@@ -152,10 +150,10 @@ jobs:
152150
runs-on: ubuntu-latest
153151
needs: [lint, test]
154152
steps:
155-
- uses: actions/checkout@v4
153+
- uses: actions/checkout@v7
156154

157155
- name: Install uv
158-
uses: astral-sh/setup-uv@v7
156+
uses: astral-sh/setup-uv@v9.0.0
159157

160158
- name: Set up Python 3.11
161159
run: |

.github/workflows/docs.yml

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,10 @@ jobs:
1919
build:
2020
runs-on: ubuntu-latest
2121
steps:
22-
- uses: actions/checkout@v4
22+
- uses: actions/checkout@v7
2323

2424
- name: Install uv
25-
uses: astral-sh/setup-uv@v7
25+
uses: astral-sh/setup-uv@v9.0.0
2626
with:
2727
enable-cache: true
2828
cache-dependency-glob: "uv.lock"
@@ -49,12 +49,17 @@ jobs:
4949
touch build/html/.nojekyll
5050
5151
- name: Setup Pages
52-
uses: actions/configure-pages@v5
52+
uses: actions/configure-pages@v6
5353

5454
- name: Upload artifact
55-
uses: actions/upload-pages-artifact@v3
55+
uses: actions/upload-pages-artifact@v5
5656
with:
5757
path: 'docs/build/html'
58+
# v4 started excluding dotfiles from the artifact. The build step writes
59+
# .nojekyll, and Sphinx output is full of _static and _sources, so losing
60+
# it is the kind of change that shows up as a broken site rather than a
61+
# failed job.
62+
include-hidden-files: true
5863

5964
deploy:
6065
environment:
@@ -66,4 +71,4 @@ jobs:
6671
steps:
6772
- name: Deploy to GitHub Pages
6873
id: deployment
69-
uses: actions/deploy-pages@v4
74+
uses: actions/deploy-pages@v5

.github/workflows/python-publish.yml

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,10 @@ jobs:
2121
test-before-publish:
2222
runs-on: ubuntu-latest
2323
steps:
24-
- uses: actions/checkout@v4
24+
- uses: actions/checkout@v7
2525

2626
- name: Install uv
27-
uses: astral-sh/setup-uv@v7
27+
uses: astral-sh/setup-uv@v9.0.0
2828
with:
2929
enable-cache: true
3030
cache-dependency-glob: "uv.lock"
@@ -48,12 +48,12 @@ jobs:
4848
url: ${{ github.event.inputs.use_test_pypi == 'true' && 'https://test.pypi.org/p/allocator' || 'https://pypi.org/p/allocator' }}
4949

5050
steps:
51-
- uses: actions/checkout@v4
51+
- uses: actions/checkout@v7
5252
with:
5353
fetch-depth: 0
5454

5555
- name: Install uv
56-
uses: astral-sh/setup-uv@v7
56+
uses: astral-sh/setup-uv@v9.0.0
5757

5858
- name: Set up Python 3.11
5959
run: |

allocator/core/routing.py

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -99,13 +99,15 @@ def solve_tsp_christofides(
9999
Returns:
100100
(total_distance, route) tuple
101101
"""
102-
try:
103-
from Christofides import christofides
104-
except ImportError as e:
105-
raise ImportError(
106-
"Christofides algorithm requires the 'Christofides' package. "
107-
"Install it with: pip install Christofides"
108-
) from e
102+
# networkx's own approximation, not the Christofides package on PyPI. That
103+
# package is Python 2 source -- importing it raises SyntaxError on `print
104+
# 'Testing...'` -- so this function had never once run on Python 3. Nothing
105+
# noticed because the only test asserted the package was *absent*, and the
106+
# CI job that installed it filtered the christofides tests out.
107+
#
108+
# networkx is already a hard dependency and has provided this since 2.6, so
109+
# there is no optional import to guard.
110+
from networkx.algorithms.approximation import christofides
109111

110112
# Get distance matrix
111113
distances = get_distance_matrix(points, points, method=distance_method, **distance_kwargs)
@@ -121,15 +123,20 @@ def solve_tsp_christofides(
121123
for j in range(i + 1, n):
122124
G.add_edge(i, j, weight=distances[i, j])
123125

124-
# Solve using Christofides algorithm
125-
tour = christofides(G, 0) # Start from node 0
126+
# A Hamiltonian cycle, so the last node repeats the first.
127+
cycle = christofides(G, weight="weight")
126128

127-
# Calculate total distance
129+
# Length of the closed tour, including the leg back to the start.
128130
total_distance = 0.0
129-
for i in range(len(tour) - 1):
130-
total_distance += distances[tour[i], tour[i + 1]]
131-
132-
return total_distance, tour
131+
for i in range(len(cycle) - 1):
132+
total_distance += distances[cycle[i], cycle[i + 1]]
133+
134+
# Return the closed cycle, first node repeated at the end, matching
135+
# solve_tsp_ortools. The two solvers are interchangeable through
136+
# shortest_path(), so a caller switching methods must not silently get a
137+
# different route convention -- and tests/api/test_route_api.py pins the
138+
# ortools one at len(points) + 1.
139+
return float(total_distance), list(cycle)
133140

134141

135142
def solve_tsp_osrm(

tests/api/test_route_api.py

Lines changed: 65 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,15 @@
22
Tests for the modern routing API.
33
"""
44

5+
import itertools
56
import unittest
67

78
import numpy as np
89
import pandas as pd
910

1011
from allocator.api import shortest_path, tsp_christofides, tsp_ortools
1112
from allocator.api.types import RouteResult
13+
from allocator.distances import get_distance_matrix
1214

1315

1416
class TestRouteAPI(unittest.TestCase):
@@ -84,12 +86,70 @@ def test_high_level_shortest_path_function(self):
8486
except ImportError:
8587
self.skipTest("OR-Tools not available")
8688

87-
def test_christofides_import_error_handling(self):
88-
"""Test Christofides handles missing dependencies gracefully."""
89-
with self.assertRaises(ImportError) as cm:
90-
tsp_christofides(self.test_points)
89+
def test_christofides_solves_a_tour(self):
90+
"""Christofides returns a tour that visits every point exactly once.
91+
92+
This replaces ``test_christofides_import_error_handling``, which asserted
93+
that ``tsp_christofides`` *raises* ImportError. That only happens when the
94+
``algorithms`` extra is absent, so the test meant one thing in the plain
95+
test job and the opposite in test-algorithms -- and it passed only
96+
because nobody had installed the extra on that runner.
97+
98+
Running it the other way is what showed the function had never worked on
99+
Python 3: the Christofides package on PyPI is Python 2 source and raises
100+
SyntaxError on import. It now uses networkx's approximation, which is a
101+
hard dependency already, so there is no optional import left to test.
102+
"""
103+
result = tsp_christofides(self.test_points)
104+
n_points = len(self.test_points)
105+
106+
self.assertIsInstance(result, RouteResult)
107+
# Closed tour, matching tsp_ortools: every point once, then back to the
108+
# start. The two solvers are interchangeable through shortest_path(), so
109+
# they must agree on what a route is.
110+
self.assertEqual(len(result.route), n_points + 1)
111+
self.assertEqual(result.route[0], result.route[-1])
112+
self.assertEqual(sorted(result.route[:-1]), list(range(n_points)))
113+
self.assertGreater(result.total_distance, 0)
114+
115+
def test_christofides_stays_within_its_approximation_guarantee(self):
116+
"""The property that makes Christofides worth using over any other tour.
117+
118+
It is a 3/2-approximation on a metric instance, so on a problem small
119+
enough to solve exactly the tour must be no worse than 1.5x optimal. A
120+
merely "valid" tour -- every point once, positive length -- would also be
121+
produced by visiting the points in input order, so without this the test
122+
above does not distinguish the algorithm from doing nothing.
123+
"""
124+
result = tsp_christofides(self.test_points)
125+
126+
# Score the tour in the metric the solver actually optimised in.
127+
# `euclidean` projects lon/lat to UTM metres before measuring
128+
# (allocator/distances/euclidean.py, utm.from_latlon), so scoring with
129+
# distances computed from raw degrees would compare a UTM-optimal tour
130+
# against a degree-optimal one. That is a different problem, and its
131+
# ratio can exceed 3/2 through projection distortion alone.
132+
points = self.test_points[["longitude", "latitude"]].to_numpy()
133+
n = len(points)
134+
distances = get_distance_matrix(points, points, method="euclidean")
135+
136+
optimal = min(
137+
sum(distances[order[i], order[i + 1]] for i in range(n - 1))
138+
+ distances[order[-1], order[0]]
139+
for order in itertools.permutations(range(n))
140+
)
141+
# result.route is the closed tour; drop the repeated start to iterate.
142+
route = result.route[:-1]
143+
tour = (
144+
sum(distances[route[i], route[i + 1]] for i in range(n - 1))
145+
+ distances[route[-1], route[0]]
146+
)
91147

92-
self.assertIn("Christofides", str(cm.exception))
148+
self.assertLessEqual(
149+
tour,
150+
1.5 * optimal,
151+
f"tour {tour:.4f} exceeds 1.5x the optimum {optimal:.4f}",
152+
)
93153

94154
def test_invalid_method(self):
95155
"""Test error handling for invalid TSP method."""

0 commit comments

Comments
 (0)