From 016d2b8c2dd8933238b88f32d4749356e1403aac Mon Sep 17 00:00:00 2001 From: saphid <4596216+saphid@users.noreply.github.com> Date: Tue, 29 Sep 2026 12:59:13 +1000 Subject: [PATCH] fix(tracking): address independent review of the pulse experiment - Watch for a second capture directory on every poll; stop and remove every directory that appeared, and fail loudly if an eye image can't be deleted. - Start the worker pool inside the cleanup block. - Flat patches no longer rank first (zero-power SNR is 0, not infinity). - Align eyes to the truly nearest frame. - Keep patch grids as float arrays (a 300 s run no longer needs ~0.4 GB). - heart-check: tolerate unusual flags, close the Health archive, give a clear error when export.xml is missing, and build the lookup index once. Found by a SWE-2 Max read-only review. Part of #27 Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/tracking.md | 8 +++--- frame/tracking/pulse.py | 55 +++++++++++++++++++++++++---------------- scripts/heart-check.py | 45 +++++++++++++++++++-------------- tests/test_pulse.py | 44 +++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 42 deletions(-) diff --git a/docs/tracking.md b/docs/tracking.md index 7f71070..f3b9c88 100644 --- a/docs/tracking.md +++ b/docs/tracking.md @@ -213,15 +213,17 @@ How it works: 42 and 180 BPM are combined in the frequency domain. Output is an overall estimate plus one estimate per second over 15-second windows. A result counts as **clear** only when the top patches agree and the combined signal - stands out from the noise. Otherwise the command exits 3 and sends nothing. + stands out from the noise. Otherwise the command exits 3 and sends no OSC. + `--log` still records the per-second estimates, so a comparison shows how + far off an unclear result was. The thresholds are provisional until checked on real wearers. **Verified on the Frame, unworn, 2026-09-29:** a 30-second run captured 5,362 eye frames, never had more than 7 images on disk, finished 3 s after the capture ended and left no capture directory. It reported no clear pulse (exit 3), as it should with nobody wearing it. Worth knowing: the unworn -patches agreed on a steady rhythm near 129 BPM (2.15 Hz) with low -signal/noise (0.19). That is a camera or illumination artifact, not a pulse, +patches agreed on a steady rhythm near 127-129 BPM (about 2.1 Hz) with low +signal/noise (0.15-0.19), in two separate runs. That is a camera or illumination artifact, not a pulse, and the signal/noise gate kept it from being reported. A worn test should also record an unworn baseline, to rule out the same artifact. diff --git a/frame/tracking/pulse.py b/frame/tracking/pulse.py index 2b582cb..1170e91 100644 --- a/frame/tracking/pulse.py +++ b/frame/tracking/pulse.py @@ -11,6 +11,7 @@ complete, then deleted; the capture directory is removed on exit. No image leaves the Frame or outlives the run. This is an experiment, not a medical measurement. """ +from array import array import bisect import cmath import json @@ -81,6 +82,7 @@ class Capture: self.pool = None self.submitted = set() self.futures = {} + self.new = set() # every capture directory that appeared during our run def candidates(self): parent, stem = os.path.split(self.PREFIX) @@ -93,23 +95,26 @@ class Capture: # found by watching for a new one rather than waiting for its name. import tempfile before = self.candidates() - if self.workers: - # SteamVR pins its eye tracker to cores 0-1; decoding runs beside it. - import concurrent.futures - import multiprocessing - self.pool = concurrent.futures.ProcessPoolExecutor( - self.workers, mp_context=multiprocessing.get_context("fork")) + process = None with tempfile.TemporaryFile("w+") as log: - process = self.runner([str(ET_BIN), "-b", "CDSP", "-w", str(ET_WEIGHTS), "--calib", str(self.seconds)], - cwd=str(ET_BIN.parent), stdout=log, stderr=subprocess.STDOUT) try: + if self.workers: + # SteamVR pins its eye tracker to cores 0-1; decoding runs beside it. + import concurrent.futures + import multiprocessing + self.pool = concurrent.futures.ProcessPoolExecutor( + self.workers, mp_context=multiprocessing.get_context("fork")) + process = self.runner([str(ET_BIN), "-b", "CDSP", "-w", str(ET_WEIGHTS), "--calib", str(self.seconds)], + cwd=str(ET_BIN.parent), stdout=log, stderr=subprocess.STDOUT) deadline = time.monotonic() + self.seconds + 30 while True: - if not self.directory: - new = self.candidates() - before - if len(new) > 1: - raise RuntimeError("another eye-camera capture is running") - self.directory = new.pop() if new else None + # Checked on every poll: a second capture directory means + # we can't tell which images are ours, so stop. + self.new |= self.candidates() - before + if len(self.new) > 1: + raise RuntimeError("another eye-camera capture is running") + if not self.directory and self.new: + self.directory = next(iter(self.new)) finished = process.poll() is not None self.reduce(final=finished) if finished: @@ -124,7 +129,7 @@ class Capture: raise RuntimeError(f"eye-camera capture failed ({reason})") return self.frames() finally: - if process.poll() is None: + if process and process.poll() is None: process.terminate() try: process.wait(timeout=5) @@ -157,10 +162,10 @@ class Capture: if self.pool: self.futures[(eye, index)] = self.pool.submit(reduce_file, self.loader, path) else: - self.grids[eye][index] = reduce_file(self.loader, path) + self.grids[eye][index] = array("f", reduce_file(self.loader, path)) for key, future in list(self.futures.items()): if final or future.done(): - self.grids[key[0]][key[1]] = future.result() + self.grids[key[0]][key[1]] = array("f", future.result()) del self.futures[key] def frames(self): @@ -177,8 +182,11 @@ class Capture: return frames def remove(self): - if self.directory and str(self.directory).startswith(self.PREFIX): - shutil.rmtree(self.directory, ignore_errors=True) + """Remove every capture directory that appeared during the run. Eye + images must not outlive it, so a failure to delete is an error.""" + for directory in self.new | ({self.directory} if self.directory else set()): + if str(directory).startswith(self.PREFIX) and directory.is_dir() and not directory.is_symlink(): + shutil.rmtree(directory) # --------------------------------------------------------------- analysis --- @@ -283,7 +291,7 @@ def snr(spec, bpm, width=4.0): """Power near the pulse and its first harmonic against the rest of the band.""" near = sum(p for f, p in spec if abs(f - bpm) <= width or abs(f - 2 * bpm) <= width) rest = sum(p for f, p in spec) - near - return near / rest if rest > 0 else float("inf") + return near / rest if near > 0 and rest > 0 else 0.0 def normalised(spec): @@ -308,7 +316,7 @@ def estimate(times, patches, rate=RATE, share=0.2, window=WINDOW): for name, values in patches.items(): if usable(values): signal = clean(resample(times, values, rate), rate) - if signal is not None: + if signal is not None and any(signal): # flat patches carry no rhythm cleaned[name] = signal if not cleaned or len(next(iter(cleaned.values()))) < rate * 8: raise ValueError("need at least 8 seconds of usable eye-camera frames") @@ -352,7 +360,12 @@ def analyse(frames): if not eye_frames: continue eye_times = [t for t, _ in eye_frames] - nearest = [min(bisect.bisect_left(eye_times, t), len(eye_times) - 1) for t in times] + nearest = [] + for t in times: + i = bisect.bisect_left(eye_times, t) + if i == len(eye_times) or (i > 0 and t - eye_times[i - 1] <= eye_times[i] - t): + i -= 1 + nearest.append(i) for k in range(len(eye_frames[0][1])): patches[f"{eye}{k}"] = [eye_frames[i][1][k] for i in nearest] return estimate(times, patches) diff --git a/scripts/heart-check.py b/scripts/heart-check.py index 1cc0fab..46de7e0 100644 --- a/scripts/heart-check.py +++ b/scripts/heart-check.py @@ -118,32 +118,39 @@ def read_csv(path): continue try: when, bpm = parse_time(row[0]), int(float(row[1])) + flags = int(float(row[2])) if len(row) > 2 and row[2].strip() else None except ValueError: continue # header or unparseable line - if len(row) > 2 and row[2].strip(): - flags = int(row[2]) - if flags & 4 and not flags & 2: - bpm = None # contact supported and not detected: no reading + if flags is not None and flags & 4 and not flags & 2: + bpm = None # contact supported and not detected: no reading samples.append((when, bpm)) return sorted(samples, key=lambda s: s[0]) def read_health(path, start, end): """Heart-rate records from an Apple Health export between start and end.""" - if zipfile.is_zipfile(path): - archive = zipfile.ZipFile(path) - name = next(n for n in archive.namelist() if n.endswith("/export.xml") or n == "export.xml") - source = archive.open(name) - else: - source = open(path, "rb") samples = [] - with source: + def scan(source): for _, element in ElementTree.iterparse(source): if element.tag == "Record" and element.get("type") == "HKQuantityTypeIdentifierHeartRate": - when = parse_time(element.get("startDate")) + try: + when = parse_time(element.get("startDate") or "") + value = round(float(element.get("value") or "")) + except ValueError: + continue if start <= when <= end: - samples.append((when, round(float(element.get("value"))))) + samples.append((when, value)) element.clear() + if zipfile.is_zipfile(path): + with zipfile.ZipFile(path) as archive: + names = [n for n in archive.namelist() if n.endswith("/export.xml") or n == "export.xml"] + if not names: + raise ValueError("no export.xml in that archive; use Health's Export All Health Data") + with archive.open(names[0]) as source: + scan(source) + else: + with open(path, "rb") as source: + scan(source) return sorted(samples) @@ -154,9 +161,10 @@ def read_any(path, start=None, end=None): return read_csv(path) -def value_at(samples, when, hold): - """Our reading at a moment: the latest sample no older than `hold` seconds.""" - times = [s[0] for s in samples] +def value_at(samples, when, hold, times=None): + """Our reading at a moment: the latest sample no older than `hold` seconds. + `times` is the samples' time column, if the caller has already built it.""" + times = times if times is not None else [s[0] for s in samples] i = bisect.bisect_right(times, when) - 1 if i < 0 or when - times[i] > hold: return None @@ -173,10 +181,11 @@ def compare(ours, reference, max_lag=10.0, hold=5.0): if not real or not any(b is not None for _, b in ours): raise ValueError("both recordings need at least one reading") best = None + ours_times = [t for t, _ in ours] steps = int(max_lag * 4) for step in range(-steps, steps + 1): lag = step / 4 - pairs = [(value_at(ours, t + lag, hold), b) for t, b in real] + pairs = [(value_at(ours, t + lag, hold, ours_times), b) for t, b in real] pairs = [(o, r) for o, r in pairs if o is not None] if not pairs: continue @@ -249,8 +258,8 @@ def main(argv=None): ours = read_csv(args.ours) if not ours: parser.error("our recording has no readings") - reference = read_any(args.reference, ours[0][0] - 60, ours[-1][0] + 60) try: + reference = read_any(args.reference, ours[0][0] - 60, ours[-1][0] + 60) result = compare(ours, reference, args.max_lag) except ValueError as error: print(str(error)) diff --git a/tests/test_pulse.py b/tests/test_pulse.py index bc2d92f..81eb91d 100644 --- a/tests/test_pulse.py +++ b/tests/test_pulse.py @@ -79,6 +79,24 @@ class Estimate(unittest.TestCase): self.assertAlmostEqual(series[0][0], pulse.WINDOW, delta=0.1) self.assertEqual(len(series), 20 - int(pulse.WINDOW) + 1) + def test_flat_patches_do_not_rank_first(self): + times, values = synthetic(72, 16) + values["flat"] = [100.0] * len(times) + result = pulse.estimate(times, values) + self.assertAlmostEqual(result["bpm"], 72, delta=1.5) + self.assertEqual(pulse.snr([(60.0, 0.0), (61.0, 0.0)], 60), 0.0) + + def test_analyse_uses_nearest_frame(self): + times, values = synthetic(72, 4, patches=4, seconds=10) + # Right-eye frames 1 ms before each left frame: nearest is that frame. + frames = [(t, "left", [values[k][i] for k in range(4)]) for i, t in enumerate(times)] + frames += [(t - 0.001, "right", [float(i)] * 4) for i, t in enumerate(times)] + seen = {} + original = pulse.estimate + with patch.object(pulse, "estimate", lambda t, p: seen.update(p) or original(t, p)): + pulse.analyse(frames) + self.assertEqual(seen["right0"][:5], [0.0, 0.0, 1.0, 1.0, 2.0]) + def test_fft_matches_dft(self): signal = [math.sin(i) + (i % 3) for i in range(16)] fast = pulse.fft(signal) @@ -213,6 +231,22 @@ class CaptureLifecycle(unittest.TestCase): self.assertEqual(tool.returncode, -15) # capture tool stopped self.assertFalse(self.directory.exists()) # no image left behind + def test_second_capture_directory_stops_and_removes_both(self): + foreign = self.root / "etcalib_foreign" + class Racing(FakeCaptureTool): + def poll(self): + if self.polls == 2: + foreign.mkdir() + (foreign / "left_0.png").write_bytes(b"png") + return super().poll() + tool = Racing(self.directory) + with self.assertRaises(RuntimeError) as raised: + self.capture(tool) + self.assertIn("another eye-camera capture", str(raised.exception)) + self.assertFalse(self.directory.exists()) + self.assertFalse(foreign.exists()) # no eye image outlives the run + self.assertTrue((self.root / "etcalib_older").exists()) + def test_only_removes_capture_directories(self): capture = pulse.Capture(20) capture.directory = self.root @@ -292,6 +326,16 @@ class HeartCheck(unittest.TestCase): result = check.compare(ours, samples) self.assertLessEqual(result["mean_abs_error"], 1) + def test_unusual_flags_and_missing_export(self): + path = self.write("ref.csv", "time,bpm,flags\n1000,70,6.0\n1001,71,yes\n1002,72,4\n") + self.assertEqual(check.read_csv(path), [(1000.0, 70), (1002.0, None)]) + with tempfile.TemporaryDirectory() as directory: + archive = Path(directory) / "other.zip" + with zipfile.ZipFile(archive, "w") as z: + z.writestr("notes.txt", "x") + with self.assertRaises(ValueError): + check.read_any(archive, 0, 1) + def test_time_formats(self): self.assertEqual(check.parse_time("1700000000.5"), 1700000000.5) self.assertEqual(check.parse_time("2023-11-14T22:13:20Z"), 1700000000)