From cfe522b913f8a0b04dab23f89918cae056cf1551 Mon Sep 17 00:00:00 2001 From: dvitale199 Date: Thu, 16 Jul 2026 16:10:04 -0600 Subject: [PATCH 1/4] docs: design spec for retiring dead abstractions Brainstormed design for removing two confirmed-orphaned Python abstractions (QCPipeline + QCResult/QCStepProtocol, and the unadopted ReferencePanel module). Deletion-only cleanup; targets refactor/main. genotools/container/ is intentionally retained as a historical reference for rebuilding containerized inference later. See the spec for evidence, per-commit plan, and verification. --- ...6-07-16-retire-dead-abstractions-design.md | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-16-retire-dead-abstractions-design.md diff --git a/docs/superpowers/specs/2026-07-16-retire-dead-abstractions-design.md b/docs/superpowers/specs/2026-07-16-retire-dead-abstractions-design.md new file mode 100644 index 0000000..a476c85 --- /dev/null +++ b/docs/superpowers/specs/2026-07-16-retire-dead-abstractions-design.md @@ -0,0 +1,92 @@ +# Design: Retire dead abstractions (refactor hardening, round 3+) + +**Date:** 2026-07-16 +**Branch target:** `refactor/main` (feature branch → PR into `refactor/main`; **never `main`**). +**Tracker item:** REFACTOR_HARDENING.md remaining-work #7 ("Retire dead abstractions"). + +## Goal + +Remove two confirmed-orphaned Python abstractions from the mid-cutover refactor so +the codebase carries less unused surface before the higher-risk legacy-decouple +work (item #6). This is a **deletion-only cleanup**: none of the removed code is +reachable at runtime, so it cannot affect the in-flight real-data parity run +(the gate for `refactor/main` → `main`, owned by another dev). + +## Why these two are safe to remove (evidence) + +1. **`QCPipeline`** (`genotools/qc/pipeline.py`) — an orchestration API + (`.run()`, `.sample_qc()`/`.variant_qc()` factories). The CLI runner + (`cli/runner.py`) reimplements orchestration in + `_run_single_step`/`_run_qc_pipeline` and **never uses `QCPipeline`**. Its + golden regression coverage is **redundant**: `tests/regression/test_qc_steps.py` + already exercises every pure step function directly against golden files + (`TestCallrateRegression`, `TestSexRegression`, `TestHetRegression`, + `TestRelatedRegression`, `TestDirectOutlierValidation`, the variant-side + equivalents, and the CLI-driven `TestAllSample/AllVariant/FullPipeline`). The + `QCPipeline` test classes only exercise the unused orchestration path. + - `QCResult` (`qc/results.py`) is produced **only** by `QCPipeline.run`; it + dies with it. `FilterResult` (returned by every step) is unrelated and stays. + - `QCStepProtocol` (`qc/pipeline.py`) is used only within that module. + +2. **`ReferencePanel`** (`genotools/ancestry/reference.py`) — a public export, + referenced only in a docstring, **never constructed in shipped code**, **zero + test references**. The whole module is dead: its two sibling helpers + `get_default_model_path()` and `validate_model_files()` are likewise only + exported and never used. The new-ancestry path uses `AncestryModel` directly. + The runner registers `_new_modules["ReferencePanel"]` but never reads it. + +**Explicitly NOT touched:** +- `genotools/container/` — although the harness (`run.py`, `Dockerfile`, bundled + `*.pkl` models, `requirements.txt`) is orphaned and not imported anywhere, it is + **intentionally retained** as a historical reference for rebuilding containerized + ancestry-inference later (maintainer decision). Its `setup.py` `package_data` + entry stays as-is. +- Verified live + tested: `ancestry/results.py` (all classes imported by + `ancestry/model.py` + tested), `qc/results.py::FilterResult`, `gwas/results.py`. + +## Plan — one PR, two commits + +### Commit 1 — `refactor: remove unused QCPipeline orchestration API` +- Delete `genotools/qc/pipeline.py` (`QCPipeline`, `QCStepProtocol`). +- Remove `QCResult` from `genotools/qc/results.py` (keep `FilterResult`). +- `genotools/qc/__init__.py`: drop `QCPipeline` and `QCResult` imports + `__all__` entries. +- Tests: + - Delete `tests/unit/test_qc/test_pipeline.py`. + - Remove `TestQCResult` from `tests/unit/test_qc/test_results.py` (keep `FilterResult` tests). + - Remove `TestQCPipelineSampleQC` / `TestQCPipelineVariantQC` (lines ~465–633) and + the `QCPipeline` imports from `tests/regression/test_qc_steps.py`. Keep all + direct step-function golden tests and the CLI-driven pipeline tests. + +### Commit 2 — `refactor: remove unadopted ReferencePanel module` +- Delete `genotools/ancestry/reference.py` in full (`ReferencePanel` + + `get_default_model_path` + `validate_model_files`). +- `genotools/ancestry/__init__.py`: remove the `from genotools.ancestry.reference import (...)` + block + the three `__all__` entries. +- `genotools/cli/runner.py`: drop `ReferencePanel` from the `from ..ancestry import ...` + line and the dead `"ReferencePanel": ReferencePanel` registration in `_new_modules`. + +## Verification (after each commit + at the end) + +1. **Static:** `grep` for every removed name — `QCPipeline`, `QCResult`, + `QCStepProtocol`, `ReferencePanel`, `get_default_model_path`, + `validate_model_files` — expect zero live references. +2. **Import smoke:** `python -c "import genotools, genotools.qc, genotools.ancestry, genotools.cli, genotools.gwas"`. +3. **Full suite:** `tests/unit tests/regression` green. Test count drops by the + deleted tests (~35–40 fewer) — deletions, not failures. +4. **Parity:** `tests/regression/test_parity.py` green with `.venv-stable` present + (proves runtime paths untouched). +5. **CLI smoke:** `python -m genotools --help`. + +## Risks / mitigations + +- *A "dead" symbol is actually referenced somewhere not grepped* → import smoke + test + full suite + step-1 grep catch it before each commit lands. +- *Interference with the parity run* → none: no QC/GWAS/ancestry execution path is + touched; changes are pure deletions of unreachable code. + +## Out of scope + +- Item #6 (decouple CLI from legacy `utils.py` / ancestry importlib hack) — the + higher-value structural work; behavior-preserving but touches the startup hot + path, so coordinate with the parity run. +- Items #9/#10 (performance) — change execution; defer until real-data parity is proven. From a5d2612711787e24fec2f2938d248f66a7024d72 Mon Sep 17 00:00:00 2001 From: dvitale199 Date: Thu, 16 Jul 2026 16:23:41 -0600 Subject: [PATCH 2/4] docs: implementation plan for retiring dead abstractions --- .../2026-07-16-retire-dead-abstractions.md | 292 ++++++++++++++++++ 1 file changed, 292 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-16-retire-dead-abstractions.md diff --git a/docs/superpowers/plans/2026-07-16-retire-dead-abstractions.md b/docs/superpowers/plans/2026-07-16-retire-dead-abstractions.md new file mode 100644 index 0000000..bbde3c2 --- /dev/null +++ b/docs/superpowers/plans/2026-07-16-retire-dead-abstractions.md @@ -0,0 +1,292 @@ +# Retire Dead Abstractions Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Delete two confirmed-orphaned Python abstractions — the unused `QCPipeline` orchestration API (with `QCResult`/`QCStepProtocol`) and the unadopted `ReferencePanel` module — from the mid-cutover refactor. + +**Architecture:** Deletion-only cleanup. None of the removed code is reachable at runtime, so correctness is proven by (a) zero remaining references, (b) imports still working, (c) the full unit+regression+parity suite staying green (minus the deleted self-tests). No production behavior changes. + +**Tech Stack:** Python 3.11, pytest, PLINK/PLINK2 (auto-downloaded), `.venv` (new code), `.venv-stable` (pre-refactor baseline for parity). + +## Global Constraints + +- Branch off `refactor/main`; PR targets **`refactor/main`, never `main`**. +- Follow CLAUDE.md: no `Co-Authored-By` line in commits; keep each concern in its own commit. +- Do NOT touch `genotools/container/` — intentionally retained as a historical reference. +- Do NOT touch `ancestry/results.py`, `qc/results.py::FilterResult`, or `gwas/results.py` — all live and tested. +- Run tests with `.venv/bin/python -m pytest`. + +--- + +### Task 1: Remove the unused QCPipeline orchestration API + +**Files:** +- Delete: `genotools/qc/pipeline.py` +- Modify: `genotools/qc/results.py` (remove `QCResult` class; keep `FilterResult`) +- Modify: `genotools/qc/__init__.py` (drop `QCPipeline`/`QCResult` imports, `__all__` entries, docstring example) +- Delete: `tests/unit/test_qc/test_pipeline.py` +- Modify: `tests/unit/test_qc/test_results.py` (remove `TestQCResult`, keep `FilterResult` tests) +- Modify: `tests/regression/test_qc_steps.py` (remove `TestQCPipelineSampleQC`/`TestQCPipelineVariantQC`) + +**Interfaces:** +- Consumes: nothing from other tasks. +- Produces: `genotools.qc` no longer exports `QCPipeline` or `QCResult`; still exports `FilterResult` and all `filter_*`/`prune_ld`/`verify_kinship` functions and configs. + +- [ ] **Step 1: Confirm nothing outside the deletion set references these symbols** + +Run: +```bash +grep -rn "QCPipeline\|QCStepProtocol" genotools/ | grep -vE "qc/pipeline.py|qc/__init__.py" +grep -rn "QCResult" genotools/ | grep -vE "qc/pipeline.py|qc/results.py|qc/__init__.py" +grep -rn "from genotools.qc.pipeline\|from genotools.qc import .*QCPipeline\|qc\.QCPipeline" genotools/ tests/ | grep -v "test_qc_steps.py\|test_pipeline.py" +``` +Expected: no output (only the files being edited/deleted reference them; `test_qc_steps.py`/`test_pipeline.py` reference QCPipeline via inline imports removed in this task). + +- [ ] **Step 2: Delete the pipeline module and its unit test** + +Run: +```bash +git rm genotools/qc/pipeline.py tests/unit/test_qc/test_pipeline.py +``` +Expected: both files staged for deletion. + +- [ ] **Step 3: Remove the `QCResult` class from `genotools/qc/results.py`** + +Delete the entire `QCResult` dataclass (the block beginning `@dataclass(frozen=True)` / `class QCResult:` through the end of `get_step_result`, i.e. from the blank lines after `FilterResult` to EOF). The file must end right after `FilterResult`'s `to_dict` method. Leave the imports (`from typing import Any, Optional` is still used by `FilterResult`) and `FilterResult` untouched. + +- [ ] **Step 4: Update `genotools/qc/__init__.py`** + +Change the import lines: +```python +from genotools.qc.pipeline import QCPipeline +from genotools.qc.results import FilterResult, QCResult +``` +to: +```python +from genotools.qc.results import FilterResult +``` + +Remove these `__all__` entries (and the now-empty `# Pipeline` comment): +```python + # Pipeline + "QCPipeline", + # Results + "QCResult", +``` +so the Results section reads: +```python + # Results + "FilterResult", +``` + +Replace the module docstring's pipeline example (the `QCPipeline(...)` / `pipeline.run(...)` block and the `QCPipeline,` line in the import example) with just the single-step usage: +```python +"""QC module for genotype quality control. + +This module provides pure functions for sample and variant quality control. + +Example usage: + from genotools.qc import filter_callrate, CallrateConfig, FilterResult + + data = GenotypeData.from_path(Path("input")) + result = filter_callrate(data, CallrateConfig(mind=0.05), Path("output")) +""" +``` + +- [ ] **Step 5: Remove `TestQCResult` from `tests/unit/test_qc/test_results.py`** + +Change the import: +```python +from genotools.qc.results import FilterResult, QCResult +``` +to: +```python +from genotools.qc.results import FilterResult +``` +Delete the entire `class TestQCResult:` block (from `class TestQCResult:` up to, but not including, `class TestLegacyCompatibility:`). Keep `TestFilterResult` and `TestLegacyCompatibility`. + +- [ ] **Step 6: Remove QCPipeline regression classes from `tests/regression/test_qc_steps.py`** + +Delete the section from the `# QCPipeline API Regression Tests` header comment through the end of `TestQCPipelineVariantQC` (i.e. `class TestQCPipelineSampleQC:` and `class TestQCPipelineVariantQC:` and their methods), up to but not including the next section header (`TestAllSamplePipeline`). The inline `from genotools.qc import QCPipeline, ...` statements live inside those methods and are removed with them. Keep every direct step-function golden test (`TestCallrateRegression` … `TestVariantOutlierValidation`) and the CLI-driven `TestAllSamplePipeline`/`TestAllVariantPipeline`/`TestFullPipeline`. + +- [ ] **Step 7: Verify no references remain and imports work** + +Run: +```bash +grep -rn "QCPipeline\|QCStepProtocol" genotools/ tests/ +grep -rn "QCResult" genotools/ tests/ +.venv/bin/python -c "import genotools, genotools.qc, genotools.ancestry, genotools.cli, genotools.gwas; from genotools.qc import FilterResult; print('imports OK')" +``` +Expected: first two greps produce no output; import prints `imports OK`. + +- [ ] **Step 8: Run the QC + regression suites** + +Run: +```bash +.venv/bin/python -m pytest tests/unit/test_qc tests/regression/test_qc_steps.py -q +``` +Expected: all pass (fewer tests than before — the deleted `QCPipeline`/`QCResult` tests are gone, not failing). + +- [ ] **Step 9: Commit** + +```bash +git add -A +git commit -m "refactor: remove unused QCPipeline orchestration API + +QCPipeline (qc/pipeline.py) plus QCResult and QCStepProtocol were an +orchestration API the CLI runner never used (it reimplements orchestration +in _run_single_step/_run_qc_pipeline). Their golden coverage was redundant +with the direct step-function golden tests, which remain. Removes the class, +its QCResult return type, their exports, and their self-tests. FilterResult +(returned by every step) is kept." +``` + +--- + +### Task 2: Remove the unadopted ReferencePanel module + +**Files:** +- Delete: `genotools/ancestry/reference.py` +- Modify: `genotools/ancestry/__init__.py` (drop the reference import block + `__all__` entries + docstring example) +- Modify: `genotools/cli/runner.py` (drop `ReferencePanel` from the ancestry import + the dead `_new_modules` registration) + +**Interfaces:** +- Consumes: nothing from Task 1. +- Produces: `genotools.ancestry` no longer exports `ReferencePanel`, `get_default_model_path`, or `validate_model_files`; the runner's `_new_modules` no longer has a `"ReferencePanel"` key (it was never read). + +- [ ] **Step 1: Confirm the module's symbols are unused outside itself** + +Run: +```bash +grep -rn "ReferencePanel\|get_default_model_path\|validate_model_files" genotools/ tests/ | grep -vE "ancestry/reference.py|ancestry/__init__.py|cli/runner.py" +``` +Expected: at most docstring hits in `ancestry/model.py` (lines with `>>>`). No real import or call. + +- [ ] **Step 2: Delete the reference module** + +Run: +```bash +git rm genotools/ancestry/reference.py +``` +Expected: file staged for deletion. + +- [ ] **Step 3: Update `genotools/ancestry/__init__.py`** + +Remove the entire reference import block: +```python +# Reference panel management +from genotools.ancestry.reference import ( + ReferencePanel, + get_default_model_path, + validate_model_files, +) +``` +Remove these `__all__` entries: +```python + "ReferencePanel", +``` +```python + "get_default_model_path", + "validate_model_files", +``` +If the module docstring contains a `ReferencePanel` example (`>>> from genotools.ancestry import ... ReferencePanel ...` and `>>> ref = ReferencePanel.load(...)`), remove those two lines from the docstring so it no longer references the deleted class. + +- [ ] **Step 4: Update `genotools/cli/runner.py`** + +Change: +```python + from ..ancestry import AncestryModel, ReferencePanel, AncestryConfig +``` +to: +```python + from ..ancestry import AncestryModel, AncestryConfig +``` +Remove the dead registration line from the `self._new_modules = {...}` dict: +```python + "ReferencePanel": ReferencePanel, +``` + +- [ ] **Step 5: Verify no references remain and imports work** + +Run: +```bash +grep -rn "ReferencePanel\|get_default_model_path\|validate_model_files" genotools/ tests/ +.venv/bin/python -c "import genotools, genotools.ancestry, genotools.cli; from genotools.ancestry import AncestryModel, AncestryConfig; print('imports OK')" +``` +Expected: grep shows at most `>>>` docstring lines in `ancestry/model.py` (harmless prose) and nothing else; import prints `imports OK`. + +- [ ] **Step 6: Run the ancestry + CLI suites** + +Run: +```bash +.venv/bin/python -m pytest tests/unit/test_ancestry tests/unit/test_cli -q +``` +Expected: all pass. + +- [ ] **Step 7: Commit** + +```bash +git add -A +git commit -m "refactor: remove unadopted ReferencePanel module + +ancestry/reference.py (ReferencePanel + get_default_model_path + +validate_model_files) was a public export referenced only in docstrings, +never constructed in shipped code, with zero test coverage. The new-ancestry +path uses AncestryModel directly, and the runner registered ReferencePanel in +_new_modules but never read it. Removes the module, its exports, and the dead +registration." +``` + +--- + +### Task 3: Full verification and PR + +**Files:** none (verification + PR only). + +**Interfaces:** +- Consumes: the two commits from Tasks 1 and 2. +- Produces: a PR into `refactor/main`. + +- [ ] **Step 1: Full unit + regression suite** + +Run: +```bash +.venv/bin/python -m pytest tests/unit tests/regression -q +``` +Expected: all pass (parity tests included when `.venv-stable` is present; they skip cleanly otherwise). Total count is lower than the 397 baseline by exactly the deleted tests — all deletions, no failures. + +- [ ] **Step 2: CLI smoke test** + +Run: +```bash +.venv/bin/python -m genotools --help +``` +Expected: usage text prints, exit code 0. + +- [ ] **Step 3: Push and open PR into refactor/main** + +```bash +git push -u origin refactor/retire-dead-abstractions +gh pr create --base refactor/main --head refactor/retire-dead-abstractions \ + --title "refactor: retire unused QCPipeline and ReferencePanel abstractions" \ + --body "" +``` +Expected: PR URL printed; `gh pr view --json baseRefName` shows `refactor/main`. + +- [ ] **Step 4: Confirm CI green** + +Run: +```bash +gh pr checks +``` +Expected: both "Unit + regression tests" and "Old-vs-new parity" pass. + +--- + +## Self-Review + +**Spec coverage:** Spec's two removal targets → Task 1 (QCPipeline/QCResult/QCStepProtocol) and Task 2 (ReferencePanel module). Spec's "explicitly NOT touched" (container/, ancestry/results.py, FilterResult, gwas/results.py) → encoded in Global Constraints and never referenced by any task. Spec's verification (grep, import smoke, full suite, parity, CLI smoke) → Task 1 steps 7–8, Task 2 steps 5–6, Task 3 steps 1–2. ✓ + +**Placeholder scan:** No TBD/TODO. The only free-text placeholder is the PR body in Task 3 Step 3 (``), which is intentionally authored at PR time. ✓ + +**Type consistency:** `FilterResult` kept everywhere; `QCResult`/`QCPipeline`/`QCStepProtocol`/`ReferencePanel`/`get_default_model_path`/`validate_model_files` removed consistently across source, exports, and tests. ✓ From 25c768b1c807905fc3ee6d8244ecdfbfdf806ccc Mon Sep 17 00:00:00 2001 From: dvitale199 Date: Thu, 16 Jul 2026 16:27:40 -0600 Subject: [PATCH 3/4] refactor: remove unused QCPipeline orchestration API QCPipeline (qc/pipeline.py) plus QCResult and QCStepProtocol were an orchestration API the CLI runner never used (it reimplements orchestration in _run_single_step/_run_qc_pipeline). Their golden coverage was redundant with the direct step-function golden tests, which remain. Removes the class, its QCResult return type, their exports, and their self-tests. FilterResult (returned by every step) is kept. --- genotools/qc/__init__.py | 23 +- genotools/qc/pipeline.py | 283 ------------------------ genotools/qc/results.py | 51 ----- tests/regression/test_qc_steps.py | 169 --------------- tests/unit/test_qc/test_pipeline.py | 325 ---------------------------- tests/unit/test_qc/test_results.py | 95 +------- 6 files changed, 3 insertions(+), 943 deletions(-) delete mode 100644 genotools/qc/pipeline.py delete mode 100644 tests/unit/test_qc/test_pipeline.py diff --git a/genotools/qc/__init__.py b/genotools/qc/__init__.py index a13ccbb..e45ccef 100644 --- a/genotools/qc/__init__.py +++ b/genotools/qc/__init__.py @@ -19,25 +19,10 @@ replacing the mutable SampleQC/VariantQC class-based architecture. Example usage: - from genotools.qc import ( - QCPipeline, - filter_callrate, - CallrateConfig, - FilterResult, - ) + from genotools.qc import filter_callrate, CallrateConfig, FilterResult - # Run a single step data = GenotypeData.from_path(Path("input")) result = filter_callrate(data, CallrateConfig(mind=0.05), Path("output")) - - # Or compose into a pipeline - pipeline = QCPipeline( - steps=[ - ("callrate", filter_callrate, CallrateConfig(mind=0.05)), - ("sex", filter_sex, SexConfig()), - ] - ) - qc_result = pipeline.run(data, Path("output_dir")) """ from genotools.qc.config import ( @@ -51,8 +36,7 @@ RelatedConfig, SexConfig, ) -from genotools.qc.pipeline import QCPipeline -from genotools.qc.results import FilterResult, QCResult +from genotools.qc.results import FilterResult from genotools.qc.steps import ( filter_callrate, filter_case_control, @@ -67,11 +51,8 @@ ) __all__ = [ - # Pipeline - "QCPipeline", # Results "FilterResult", - "QCResult", # Configs "CallrateConfig", "CaseControlConfig", diff --git a/genotools/qc/pipeline.py b/genotools/qc/pipeline.py deleted file mode 100644 index 5410924..0000000 --- a/genotools/qc/pipeline.py +++ /dev/null @@ -1,283 +0,0 @@ -# Copyright 2023 The GenoTools Authors. All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -# ============================================================================== - -"""QC pipeline orchestration. - -Composes QC steps into a sequential pipeline with configurable -error handling. -""" - -import logging -from dataclasses import dataclass -from pathlib import Path -from typing import Any, Callable, Protocol - -from genotools.core.exceptions import QCError -from genotools.core.genotypes import GenotypeData -from genotools.core.logging import step_context -from genotools.qc.results import FilterResult, QCResult - -logger = logging.getLogger(__name__) - - -class QCStepProtocol(Protocol): - """Protocol for QC step functions. - - All QC step functions must accept: - - data: GenotypeData (input) - - config: Step-specific configuration - - out_path: Path for output files - - And return a FilterResult. - """ - - def __call__( - self, - data: GenotypeData, - config: Any, - out_path: Path, - ) -> FilterResult: ... - - -# Type alias for step tuple -QCStep = tuple[str, Callable[..., FilterResult], Any] - - -@dataclass -class QCPipeline: - """Composes QC steps into a sequential pipeline. - - The pipeline executes steps in order, passing the output of each - step as input to the next. Handles error recovery based on - warn_only setting. - - Attributes: - steps: List of (name, step_function, config) tuples. - warn_only: If True, continue on step failure (--warn flag behavior). - Failed steps are skipped and the previous data is passed forward. - - Example: - from genotools.qc import ( - QCPipeline, - filter_callrate, - filter_sex, - CallrateConfig, - SexConfig, - ) - - pipeline = QCPipeline( - steps=[ - ("callrate", filter_callrate, CallrateConfig(mind=0.05)), - ("sex", filter_sex, SexConfig()), - ], - warn_only=False, - ) - result = pipeline.run(data, Path("output")) - """ - - steps: list[QCStep] - warn_only: bool = False - - def run(self, data: GenotypeData, out_dir: Path) -> QCResult: - """Execute all steps sequentially. - - Args: - data: Input genotype data. - out_dir: Directory for step outputs. Each step creates a - subdirectory, with the final output at out_dir/output. - - Returns: - QCResult with all step results and final filtered data. - - Raises: - QCError: If a step fails and warn_only is False, or if - all samples/variants are removed. - """ - out_dir.mkdir(parents=True, exist_ok=True) - - current = data - step_results: list[tuple[str, FilterResult]] = [] - - logger.info( - f"Starting QC pipeline with {len(self.steps)} steps " - f"on {data.sample_count} samples, {data.variant_count} variants" - ) - - for i, (name, step_fn, config) in enumerate(self.steps): - is_last_step = i == len(self.steps) - 1 - - with step_context(name): - logger.info(f"Starting step {i + 1}/{len(self.steps)}: {name}") - - # Determine output path - if is_last_step: - step_out = out_dir / "output" - else: - step_out = out_dir / name / "output" - step_out.parent.mkdir(parents=True, exist_ok=True) - - try: - result = step_fn(current, config, step_out) - step_results.append((name, result)) - - logger.info( - f"Completed {name}: " - f"-{result.samples_removed} samples, " - f"-{result.variants_removed} variants " - f"({result.output.sample_count} samples, " - f"{result.output.variant_count} variants remaining)" - ) - - current = result.output - - # Check for empty data - if current.sample_count == 0: - raise QCError(f"All samples removed at step: {name}") - if current.variant_count == 0: - raise QCError(f"All variants removed at step: {name}") - - except QCError as e: - if self.warn_only: - logger.warning(f"Step {name} failed (--warn mode): {e}") - # Continue with previous data - continue - else: - raise - - total_samples_removed = data.sample_count - current.sample_count - total_variants_removed = data.variant_count - current.variant_count - - logger.info( - f"QC pipeline complete: " - f"removed {total_samples_removed} samples, {total_variants_removed} variants " - f"({current.sample_count} samples, {current.variant_count} variants remaining)" - ) - - return QCResult( - input=data, - output=current, - step_results=step_results, - ) - - def add_step( - self, - name: str, - step_fn: Callable[..., FilterResult], - config: Any, - ) -> "QCPipeline": - """Add a step to the pipeline. - - Returns a new QCPipeline with the additional step. - Does not modify the original pipeline. - - Args: - name: Step name for logging and result tracking. - step_fn: Step function matching QCStepProtocol. - config: Configuration for the step. - - Returns: - New QCPipeline with the step added. - """ - return QCPipeline( - steps=self.steps + [(name, step_fn, config)], - warn_only=self.warn_only, - ) - - @classmethod - def sample_qc( - cls, - callrate_config: Any = None, - sex_config: Any = None, - het_config: Any = None, - related_config: Any = None, - warn_only: bool = False, - ) -> "QCPipeline": - """Create a standard sample QC pipeline. - - Args: - callrate_config: CallrateConfig, or None to skip. - sex_config: SexConfig, or None to skip. - het_config: HetConfig, or None to skip. - related_config: RelatedConfig, or None to skip. - warn_only: If True, continue on step failure. - - Returns: - QCPipeline configured for sample QC. - """ - from genotools.qc.steps import ( - filter_callrate, - filter_heterozygosity, - filter_relatedness, - filter_sex, - ) - - steps: list[QCStep] = [] - - if callrate_config is not None: - steps.append(("callrate", filter_callrate, callrate_config)) - if sex_config is not None: - steps.append(("sex", filter_sex, sex_config)) - if het_config is not None: - steps.append(("het", filter_heterozygosity, het_config)) - if related_config is not None: - steps.append(("related", filter_relatedness, related_config)) - - return cls(steps=steps, warn_only=warn_only) - - @classmethod - def variant_qc( - cls, - geno_config: Any = None, - case_control_config: Any = None, - haplotype_config: Any = None, - hwe_config: Any = None, - ld_config: Any = None, - warn_only: bool = False, - ) -> "QCPipeline": - """Create a standard variant QC pipeline. - - Args: - geno_config: GenoConfig, or None to skip. - case_control_config: CaseControlConfig, or None to skip. - haplotype_config: HaplotypeConfig, or None to skip. - hwe_config: HWEConfig, or None to skip. - ld_config: LDConfig, or None to skip. - warn_only: If True, continue on step failure. - - Returns: - QCPipeline configured for variant QC. - """ - from genotools.qc.steps import ( - filter_case_control, - filter_haplotype, - filter_hwe, - filter_variant_missingness, - prune_ld, - ) - - steps: list[QCStep] = [] - - if geno_config is not None: - steps.append(("geno", filter_variant_missingness, geno_config)) - if case_control_config is not None: - steps.append(("case_control", filter_case_control, case_control_config)) - if haplotype_config is not None: - steps.append(("haplotype", filter_haplotype, haplotype_config)) - if hwe_config is not None: - steps.append(("hwe", filter_hwe, hwe_config)) - if ld_config is not None: - steps.append(("ld", prune_ld, ld_config)) - - return cls(steps=steps, warn_only=warn_only) diff --git a/genotools/qc/results.py b/genotools/qc/results.py index 7e25e6d..d86323c 100644 --- a/genotools/qc/results.py +++ b/genotools/qc/results.py @@ -96,54 +96,3 @@ def to_dict(self) -> dict[str, Any]: "metrics": metrics_dict, "output": output_dict, } - - -@dataclass(frozen=True) -class QCResult: - """Result of running the full QC pipeline. - - This captures the complete result of running multiple QC steps, - including the final filtered data and results from each step. - - Attributes: - input: Original input GenotypeData before any filtering. - output: Final filtered GenotypeData after all steps. - step_results: List of (step_name, FilterResult) tuples in execution order. - """ - - input: GenotypeData - output: GenotypeData - step_results: list[tuple[str, FilterResult]] - - @property - def total_samples_removed(self) -> int: - """Total number of samples removed across all steps.""" - return sum(r.samples_removed for _, r in self.step_results) - - @property - def total_variants_removed(self) -> int: - """Total number of variants removed across all steps.""" - return sum(r.variants_removed for _, r in self.step_results) - - def to_legacy_dict(self) -> dict[str, dict[str, Any]]: - """Convert to legacy pass_fail dictionary format. - - Returns: - Dictionary mapping step names to their result dictionaries, - matching the format used by the old pipeline code. - """ - return {name: result.to_dict() for name, result in self.step_results} - - def get_step_result(self, step_name: str) -> Optional[FilterResult]: - """Get the result for a specific step by name. - - Args: - step_name: Name of the step to retrieve. - - Returns: - FilterResult for the step, or None if not found. - """ - for name, result in self.step_results: - if name == step_name: - return result - return None diff --git a/tests/regression/test_qc_steps.py b/tests/regression/test_qc_steps.py index 6c1b99b..e851337 100644 --- a/tests/regression/test_qc_steps.py +++ b/tests/regression/test_qc_steps.py @@ -462,175 +462,6 @@ def test_geno_miss_variants_detected( ) -# ============================================================================= -# QCPipeline API Regression Tests (New Implementation) -# ============================================================================= - - -class TestQCPipelineSampleQC: - """Regression tests for QCPipeline.sample_qc() against golden output. - - These tests verify the NEW QCPipeline API produces the same results - as the old SampleQC class-based implementation. - """ - - def test_sample_qc_pipeline_matches_golden( - self, test_geno_path: Path, golden_all_sample: Path, tmp_output_dir: Path - ): - """QCPipeline.sample_qc() produces same final output as golden. - - This is the key regression test for Phase 2 completion. - """ - from genotools.core.genotypes import GenotypeData - from genotools.qc import ( - QCPipeline, - CallrateConfig, - SexConfig, - HetConfig, - RelatedConfig, - ) - - # Load input data - data = GenotypeData.from_path(test_geno_path) - - # Create pipeline with same settings as --all_sample - pipeline = QCPipeline.sample_qc( - callrate_config=CallrateConfig(mind=0.05), - sex_config=SexConfig(), - het_config=HetConfig(), - related_config=RelatedConfig(related_cutoff=0.0884), - ) - - # Run the pipeline - output_dir = tmp_output_dir / "sample_qc_pipeline" - result = pipeline.run(data, output_dir) - - # Compare final output to golden - golden_output = golden_all_sample / "output" - comparison = compare_pfiles(golden_output, result.output.path) - - assert comparison.equal, ( - f"QCPipeline output differs from golden:\n" - f" Sample diff: {comparison.sample_diff}\n" - f" Variant diff: {comparison.variant_diff}\n" - f" {comparison.message}" - ) - - def test_sample_qc_pipeline_sample_counts_match( - self, test_geno_path: Path, golden_all_sample: Path, tmp_output_dir: Path - ): - """QCPipeline removes the same number of samples as golden.""" - from genotools.core.genotypes import GenotypeData - from genotools.qc import ( - QCPipeline, - CallrateConfig, - SexConfig, - HetConfig, - RelatedConfig, - ) - - # Load golden output to get expected counts - golden_output = golden_all_sample / "output" - golden_data = GenotypeData.from_path(golden_output) - - # Load input and run pipeline - data = GenotypeData.from_path(test_geno_path) - pipeline = QCPipeline.sample_qc( - callrate_config=CallrateConfig(mind=0.05), - sex_config=SexConfig(), - het_config=HetConfig(), - related_config=RelatedConfig(related_cutoff=0.0884), - ) - - output_dir = tmp_output_dir / "sample_qc_counts" - result = pipeline.run(data, output_dir) - - assert result.output.sample_count == golden_data.sample_count, ( - f"Sample count mismatch: " - f"expected {golden_data.sample_count}, got {result.output.sample_count}" - ) - - def test_sample_qc_pipeline_is_deterministic( - self, test_geno_path: Path, tmp_output_dir: Path - ): - """QCPipeline.sample_qc() produces identical output on repeated runs.""" - from genotools.core.genotypes import GenotypeData - from genotools.qc import ( - QCPipeline, - CallrateConfig, - SexConfig, - HetConfig, - RelatedConfig, - ) - - data = GenotypeData.from_path(test_geno_path) - pipeline = QCPipeline.sample_qc( - callrate_config=CallrateConfig(mind=0.05), - sex_config=SexConfig(), - het_config=HetConfig(), - related_config=RelatedConfig(related_cutoff=0.0884), - ) - - # Run twice - result1 = pipeline.run(data, tmp_output_dir / "run1") - result2 = pipeline.run(data, tmp_output_dir / "run2") - - comparison = compare_pfiles(result1.output.path, result2.output.path) - - assert comparison.equal, ( - f"Pipeline is not deterministic:\n" - f" Sample diff: {comparison.sample_diff}\n" - f" {comparison.message}" - ) - - -class TestQCPipelineVariantQC: - """Regression tests for QCPipeline.variant_qc() against golden output.""" - - def test_variant_qc_pipeline_matches_golden( - self, golden_related: Path, golden_all_variant: Path, tmp_output_dir: Path - ): - """QCPipeline.variant_qc() produces same final output as golden. - - Note: Variant QC runs on sample-QC-filtered data (related output). - """ - from genotools.core.genotypes import GenotypeData - from genotools.qc import ( - QCPipeline, - GenoConfig, - CaseControlConfig, - HaplotypeConfig, - HWEConfig, - ) - - # Input is the sample-QC-filtered output - related_output = golden_related / "output" - data = GenotypeData.from_path(related_output) - - # Create pipeline with same settings as --all_variant - pipeline = QCPipeline.variant_qc( - geno_config=GenoConfig(geno=0.05), - hwe_config=HWEConfig(hwe_threshold=1e-4), - case_control_config=CaseControlConfig(p_threshold=1e-4), - haplotype_config=HaplotypeConfig(), - ) - - # Run the pipeline - output_dir = tmp_output_dir / "variant_qc_pipeline" - result = pipeline.run(data, output_dir) - - # Compare final output to golden - golden_output = golden_all_variant / "output" - comparison = compare_pfiles(golden_output, result.output.path) - - assert comparison.equal, ( - f"QCPipeline output differs from golden:\n" - f" Sample diff: {comparison.sample_diff}\n" - f" Variant diff: {comparison.variant_diff}\n" - f" {comparison.message}" - ) - - # ============================================================================= # CLI Pipeline Integration Tests (Old CLI - will pass after Phase 4) # ============================================================================= diff --git a/tests/unit/test_qc/test_pipeline.py b/tests/unit/test_qc/test_pipeline.py deleted file mode 100644 index 162b5a6..0000000 --- a/tests/unit/test_qc/test_pipeline.py +++ /dev/null @@ -1,325 +0,0 @@ -# Copyright 2023 The GenoTools Authors. All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -# ============================================================================== - -"""Tests for QC pipeline orchestration.""" - -from pathlib import Path -from typing import Any -from unittest.mock import MagicMock, patch - -import pytest - -from genotools.core.exceptions import QCError -from genotools.qc.config import CallrateConfig, GenoConfig, SexConfig -from genotools.qc.pipeline import QCPipeline -from genotools.qc.results import FilterResult - - -class TestQCPipeline: - """Tests for QCPipeline class.""" - - @pytest.fixture - def mock_genotype_data(self) -> MagicMock: - """Create a mock GenotypeData object.""" - mock = MagicMock() - mock.path = Path("/input/test") - mock.sample_count = 500 - mock.variant_count = 10000 - mock.pgen = MagicMock() - mock.pgen.exists.return_value = True - return mock - - @pytest.fixture - def mock_step_fn(self, mock_genotype_data: MagicMock) -> MagicMock: - """Create a mock step function.""" - - def create_output(sample_count: int, variant_count: int) -> MagicMock: - output = MagicMock() - output.path = Path("/output/step") - output.sample_count = sample_count - output.variant_count = variant_count - return output - - def step_fn( - data: Any, config: Any, out_path: Path - ) -> FilterResult: - output = create_output( - data.sample_count - 10, # Remove 10 samples - data.variant_count, - ) - return FilterResult( - output=output, - samples_removed=10, - variants_removed=0, - metrics={}, - log="", - pruned_samples_file=None, - step_name="test_step", - ) - - return MagicMock(side_effect=step_fn) - - def test_empty_pipeline(self, mock_genotype_data: MagicMock, tmp_path: Path) -> None: - """Empty pipeline returns input data unchanged.""" - pipeline = QCPipeline(steps=[]) - result = pipeline.run(mock_genotype_data, tmp_path) - - assert result.input == mock_genotype_data - assert result.output == mock_genotype_data - assert len(result.step_results) == 0 - - def test_single_step( - self, mock_genotype_data: MagicMock, mock_step_fn: MagicMock, tmp_path: Path - ) -> None: - """Single step pipeline executes correctly.""" - pipeline = QCPipeline( - steps=[("test_step", mock_step_fn, CallrateConfig())] - ) - result = pipeline.run(mock_genotype_data, tmp_path) - - assert mock_step_fn.called - assert len(result.step_results) == 1 - assert result.step_results[0][0] == "test_step" - - def test_multiple_steps( - self, mock_genotype_data: MagicMock, tmp_path: Path - ) -> None: - """Multiple steps are chained correctly.""" - step_calls = [] - - def make_step(name: str) -> MagicMock: - def step_fn(data: Any, config: Any, out_path: Path) -> FilterResult: - step_calls.append(name) - output = MagicMock() - output.path = out_path - output.sample_count = data.sample_count - 5 - output.variant_count = data.variant_count - return FilterResult( - output=output, - samples_removed=5, - variants_removed=0, - metrics={}, - log="", - pruned_samples_file=None, - step_name=name, - ) - - return MagicMock(side_effect=step_fn) - - step1 = make_step("step1") - step2 = make_step("step2") - step3 = make_step("step3") - - pipeline = QCPipeline( - steps=[ - ("step1", step1, CallrateConfig()), - ("step2", step2, SexConfig()), - ("step3", step3, GenoConfig()), - ] - ) - result = pipeline.run(mock_genotype_data, tmp_path) - - assert step_calls == ["step1", "step2", "step3"] - assert len(result.step_results) == 3 - assert result.total_samples_removed == 15 # 5 + 5 + 5 - - def test_warn_only_continues_on_error( - self, mock_genotype_data: MagicMock, tmp_path: Path - ) -> None: - """warn_only=True continues pipeline after step failure.""" - step_calls = [] - - def failing_step(data: Any, config: Any, out_path: Path) -> FilterResult: - step_calls.append("failing") - raise QCError("Step failed") - - def passing_step(data: Any, config: Any, out_path: Path) -> FilterResult: - step_calls.append("passing") - output = MagicMock() - output.path = out_path - output.sample_count = data.sample_count - output.variant_count = data.variant_count - return FilterResult( - output=output, - samples_removed=0, - variants_removed=0, - metrics={}, - log="", - pruned_samples_file=None, - step_name="passing", - ) - - pipeline = QCPipeline( - steps=[ - ("failing", failing_step, CallrateConfig()), - ("passing", passing_step, SexConfig()), - ], - warn_only=True, - ) - result = pipeline.run(mock_genotype_data, tmp_path) - - assert step_calls == ["failing", "passing"] - assert len(result.step_results) == 1 # Only passing step has result - assert result.step_results[0][0] == "passing" - - def test_warn_only_false_raises_on_error( - self, mock_genotype_data: MagicMock, tmp_path: Path - ) -> None: - """warn_only=False raises exception on step failure.""" - - def failing_step(data: Any, config: Any, out_path: Path) -> FilterResult: - raise QCError("Step failed") - - pipeline = QCPipeline( - steps=[("failing", failing_step, CallrateConfig())], - warn_only=False, - ) - - with pytest.raises(QCError, match="Step failed"): - pipeline.run(mock_genotype_data, tmp_path) - - def test_raises_when_all_samples_removed( - self, mock_genotype_data: MagicMock, tmp_path: Path - ) -> None: - """Raises QCError when all samples are removed.""" - - def remove_all_step(data: Any, config: Any, out_path: Path) -> FilterResult: - output = MagicMock() - output.path = out_path - output.sample_count = 0 # All samples removed - output.variant_count = data.variant_count - return FilterResult( - output=output, - samples_removed=data.sample_count, - variants_removed=0, - metrics={}, - log="", - pruned_samples_file=None, - step_name="remove_all", - ) - - pipeline = QCPipeline( - steps=[("remove_all", remove_all_step, CallrateConfig())] - ) - - with pytest.raises(QCError, match="All samples removed"): - pipeline.run(mock_genotype_data, tmp_path) - - def test_raises_when_all_variants_removed( - self, mock_genotype_data: MagicMock, tmp_path: Path - ) -> None: - """Raises QCError when all variants are removed.""" - - def remove_all_step(data: Any, config: Any, out_path: Path) -> FilterResult: - output = MagicMock() - output.path = out_path - output.sample_count = data.sample_count - output.variant_count = 0 # All variants removed - return FilterResult( - output=output, - samples_removed=0, - variants_removed=data.variant_count, - metrics={}, - log="", - pruned_samples_file=None, - step_name="remove_all", - ) - - pipeline = QCPipeline( - steps=[("remove_all", remove_all_step, GenoConfig())] - ) - - with pytest.raises(QCError, match="All variants removed"): - pipeline.run(mock_genotype_data, tmp_path) - - def test_add_step_returns_new_pipeline(self) -> None: - """add_step returns a new pipeline, doesn't modify original.""" - original = QCPipeline(steps=[]) - - def dummy_step(data: Any, config: Any, out_path: Path) -> FilterResult: - raise NotImplementedError - - new_pipeline = original.add_step("new_step", dummy_step, CallrateConfig()) - - assert len(original.steps) == 0 - assert len(new_pipeline.steps) == 1 - assert new_pipeline is not original - - -class TestQCPipelineFactories: - """Tests for QCPipeline factory methods.""" - - def test_sample_qc_all_steps(self) -> None: - """sample_qc creates pipeline with all steps when all configs provided.""" - pipeline = QCPipeline.sample_qc( - callrate_config=CallrateConfig(), - sex_config=SexConfig(), - het_config=MagicMock(), - related_config=MagicMock(), - ) - - assert len(pipeline.steps) == 4 - step_names = [name for name, _, _ in pipeline.steps] - assert step_names == ["callrate", "sex", "het", "related"] - - def test_sample_qc_some_steps(self) -> None: - """sample_qc creates pipeline with only specified steps.""" - pipeline = QCPipeline.sample_qc( - callrate_config=CallrateConfig(), - sex_config=None, - het_config=MagicMock(), - related_config=None, - ) - - assert len(pipeline.steps) == 2 - step_names = [name for name, _, _ in pipeline.steps] - assert step_names == ["callrate", "het"] - - def test_sample_qc_warn_only(self) -> None: - """sample_qc respects warn_only parameter.""" - pipeline = QCPipeline.sample_qc( - callrate_config=CallrateConfig(), - warn_only=True, - ) - - assert pipeline.warn_only is True - - def test_variant_qc_all_steps(self) -> None: - """variant_qc creates pipeline with all steps when all configs provided.""" - pipeline = QCPipeline.variant_qc( - geno_config=GenoConfig(), - case_control_config=MagicMock(), - haplotype_config=MagicMock(), - hwe_config=MagicMock(), - ld_config=MagicMock(), - ) - - assert len(pipeline.steps) == 5 - step_names = [name for name, _, _ in pipeline.steps] - assert step_names == ["geno", "case_control", "haplotype", "hwe", "ld"] - - def test_variant_qc_some_steps(self) -> None: - """variant_qc creates pipeline with only specified steps.""" - pipeline = QCPipeline.variant_qc( - geno_config=GenoConfig(), - case_control_config=None, - haplotype_config=None, - hwe_config=MagicMock(), - ld_config=None, - ) - - assert len(pipeline.steps) == 2 - step_names = [name for name, _, _ in pipeline.steps] - assert step_names == ["geno", "hwe"] diff --git a/tests/unit/test_qc/test_results.py b/tests/unit/test_qc/test_results.py index da1f873..73deeda 100644 --- a/tests/unit/test_qc/test_results.py +++ b/tests/unit/test_qc/test_results.py @@ -20,7 +20,7 @@ import pytest -from genotools.qc.results import FilterResult, QCResult +from genotools.qc.results import FilterResult class TestFilterResult: @@ -96,99 +96,6 @@ def test_frozen(self, sample_filter_result: FilterResult) -> None: sample_filter_result.samples_removed = 0 # type: ignore[misc] -class TestQCResult: - """Tests for QCResult dataclass.""" - - @pytest.fixture - def mock_input_data(self) -> MagicMock: - """Create mock input GenotypeData.""" - mock = MagicMock() - mock.path = Path("/input/test") - mock.sample_count = 500 - mock.variant_count = 10000 - return mock - - @pytest.fixture - def mock_output_data(self) -> MagicMock: - """Create mock output GenotypeData.""" - mock = MagicMock() - mock.path = Path("/output/test") - mock.sample_count = 450 - mock.variant_count = 9000 - return mock - - @pytest.fixture - def sample_qc_result( - self, - mock_input_data: MagicMock, - mock_output_data: MagicMock, - ) -> QCResult: - """Create a sample QCResult for testing.""" - step1_output = MagicMock() - step1_output.path = Path("/output/step1") - step1_output.sample_count = 480 - - step1_result = FilterResult( - output=step1_output, - samples_removed=20, - variants_removed=0, - metrics={}, - log="", - pruned_samples_file=Path("/output/step1.outliers"), - step_name="step1", - ) - - step2_result = FilterResult( - output=mock_output_data, - samples_removed=30, - variants_removed=1000, - metrics={}, - log="", - pruned_samples_file=Path("/output/step2.outliers"), - step_name="step2", - ) - - return QCResult( - input=mock_input_data, - output=mock_output_data, - step_results=[("step1", step1_result), ("step2", step2_result)], - ) - - def test_total_samples_removed(self, sample_qc_result: QCResult) -> None: - """total_samples_removed sums across all steps.""" - assert sample_qc_result.total_samples_removed == 50 # 20 + 30 - - def test_total_variants_removed(self, sample_qc_result: QCResult) -> None: - """total_variants_removed sums across all steps.""" - assert sample_qc_result.total_variants_removed == 1000 # 0 + 1000 - - def test_to_legacy_dict(self, sample_qc_result: QCResult) -> None: - """to_legacy_dict returns correct format.""" - legacy_dict = sample_qc_result.to_legacy_dict() - - assert "step1" in legacy_dict - assert "step2" in legacy_dict - assert legacy_dict["step1"]["step"] == "step1" - assert legacy_dict["step2"]["step"] == "step2" - - def test_get_step_result_found(self, sample_qc_result: QCResult) -> None: - """get_step_result returns correct result when found.""" - result = sample_qc_result.get_step_result("step1") - assert result is not None - assert result.step_name == "step1" - assert result.samples_removed == 20 - - def test_get_step_result_not_found(self, sample_qc_result: QCResult) -> None: - """get_step_result returns None when not found.""" - result = sample_qc_result.get_step_result("nonexistent") - assert result is None - - def test_frozen(self, sample_qc_result: QCResult) -> None: - """QCResult is immutable.""" - with pytest.raises(AttributeError): - sample_qc_result.output = MagicMock() # type: ignore[misc] - - class TestLegacyCompatibility: """Tests ensuring backward compatibility with old return format.""" From 012ecb4edf670a4c22ee28c6be96f50ec01eb4d7 Mon Sep 17 00:00:00 2001 From: dvitale199 Date: Thu, 16 Jul 2026 16:29:58 -0600 Subject: [PATCH 4/4] refactor: remove unadopted ReferencePanel module ancestry/reference.py (ReferencePanel + get_default_model_path + validate_model_files) was a public export referenced only in docstrings, never constructed in shipped code, with zero test coverage. The new-ancestry path uses AncestryModel directly, and the runner registered ReferencePanel in _new_modules but never read it. Removes the module, its exports, and the dead registration. Also fixes the AncestryModel docstrings, which showed a stale ReferencePanel-based workflow (fit() actually takes a DataFrame + labels). --- genotools/ancestry/__init__.py | 22 +- genotools/ancestry/model.py | 17 +- genotools/ancestry/reference.py | 351 -------------------------------- genotools/cli/runner.py | 3 +- 4 files changed, 10 insertions(+), 383 deletions(-) delete mode 100644 genotools/ancestry/reference.py diff --git a/genotools/ancestry/__init__.py b/genotools/ancestry/__init__.py index 291bbae..8d87a69 100644 --- a/genotools/ancestry/__init__.py +++ b/genotools/ancestry/__init__.py @@ -19,17 +19,11 @@ using PCA, UMAP, and XGBoost classification. Basic Usage: - >>> from genotools.ancestry import AncestryModel, ReferencePanel, AncestryConfig + >>> from genotools.ancestry import AncestryModel, AncestryConfig >>> - >>> # Load reference panel - >>> ref = ReferencePanel.load( - ... geno_path=Path("/path/to/ref_panel"), - ... labels_path=Path("/path/to/labels.txt") - ... ) - >>> - >>> # Create and train model + >>> # Create and train model on reference genotypes + labels >>> model = AncestryModel() - >>> model.fit(ref) + >>> model.fit(raw_ref_data, labels) >>> >>> # Predict ancestry >>> predictions = model.predict(test_data) @@ -91,13 +85,6 @@ SplitResult, ) -# Reference panel management -from genotools.ancestry.reference import ( - ReferencePanel, - get_default_model_path, - validate_model_files, -) - # Main model class from genotools.ancestry.model import ( AncestryModel, @@ -117,7 +104,6 @@ __all__ = [ # Main classes "AncestryModel", - "ReferencePanel", # Configuration "AncestryConfig", "PCAConfig", @@ -137,8 +123,6 @@ "UMAPResult", "SplitResult", # Utilities - "get_default_model_path", - "validate_model_files", "load_trained_pipeline", # Reducers "PCAReducer", diff --git a/genotools/ancestry/model.py b/genotools/ancestry/model.py index a618ba2..ce6cef4 100644 --- a/genotools/ancestry/model.py +++ b/genotools/ancestry/model.py @@ -24,14 +24,10 @@ Example: >>> from genotools.ancestry.model import AncestryModel - >>> from genotools.ancestry.reference import ReferencePanel >>> - >>> # Load reference panel - >>> ref = ReferencePanel.load(ref_path, labels_path) - >>> - >>> # Create and train model + >>> # Create and train model on reference genotypes + labels >>> model = AncestryModel() - >>> model.fit(ref) + >>> model.fit(raw_ref_data, labels) >>> >>> # Predict ancestry >>> predictions = model.predict(test_data) @@ -91,11 +87,10 @@ class AncestryModel: The model combines PCA, UMAP, and XGBoost into a prediction pipeline. The typical workflow is: - 1. Load reference panel with ReferencePanel.load() - 2. Create model: AncestryModel(config) - 3. Fit on reference: model.fit(reference) - 4. Predict on new samples: model.predict(new_data) - 5. Optionally save: model.save(path) + 1. Create model: AncestryModel(config) + 2. Fit on reference genotypes + labels: model.fit(raw_ref_data, labels) + 3. Predict on new samples: model.predict(new_data) + 4. Optionally save: model.save(path) Attributes: config: Full ancestry configuration. diff --git a/genotools/ancestry/reference.py b/genotools/ancestry/reference.py deleted file mode 100644 index 7331480..0000000 --- a/genotools/ancestry/reference.py +++ /dev/null @@ -1,351 +0,0 @@ -# Copyright 2023 The GenoTools Authors. All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -# ============================================================================== - -"""Reference panel management for ancestry prediction. - -This module provides the ReferencePanel class for loading and managing -reference population data used in ancestry prediction. Reference panels -contain genotype data from individuals with known ancestry labels. - -Example: - >>> from genotools.ancestry.reference import ReferencePanel - >>> ref = ReferencePanel.load( - ... geno_path=Path("/path/to/ref_panel"), - ... labels_path=Path("/path/to/labels.txt") - ... ) - >>> ref.sample_count - 3000 - >>> ref.ancestry_counts - {'EUR': 500, 'AFR': 500, ...} -""" - -from dataclasses import dataclass -from pathlib import Path -from typing import Dict, List, Optional, Set - -import pandas as pd - -from genotools.core.genotypes import GenotypeData -from genotools.core.exceptions import AncestryError - - -@dataclass -class ReferencePanel: - """Reference panel for ancestry prediction. - - A reference panel contains genotype data from individuals with known - ancestry labels. This data is used to train the ancestry prediction - model and provide a basis for projecting new samples. - - The reference panel expects: - - Genotype files in PLINK bfile format (.bed/.bim/.fam) - - A labels file with tab-separated FID, IID, label columns - (no header, or header with these column names) - - Attributes: - genotypes: GenotypeData pointing to the reference genotype files. - labels: Series mapping sample IID to ancestry label. - labels_path: Path to the labels file that was loaded. - """ - - genotypes: GenotypeData - labels: pd.Series # type: ignore[type-arg] - labels_path: Path - - def __post_init__(self) -> None: - """Validate reference panel data.""" - if len(self.labels) == 0: - raise AncestryError("Reference panel has no labeled samples") - - # Check that labels index matches sample IDs in genotype data - # This is validated at load time, but we double-check here - - @classmethod - def load( - cls, - geno_path: Path, - labels_path: Path, - require_all_labeled: bool = True, - ) -> "ReferencePanel": - """Load reference panel from files. - - Args: - geno_path: Path prefix to genotype files (without extension). - Expects bfile format (.bed/.bim/.fam). - labels_path: Path to ancestry labels file. Tab-separated with - columns: FID, IID, label (or no header with same order). - require_all_labeled: If True, raise error if any samples in - the genotype files lack labels. Default is True. - - Returns: - Loaded ReferencePanel instance. - - Raises: - FileNotFoundError: If genotype or labels files don't exist. - AncestryError: If labels are invalid or don't match samples. - """ - geno_path = Path(geno_path) - labels_path = Path(labels_path) - - # Validate genotype files exist - if not geno_path.with_suffix(".bed").exists(): - raise FileNotFoundError( - f"Reference panel bed file not found: {geno_path}.bed" - ) - if not geno_path.with_suffix(".bim").exists(): - raise FileNotFoundError( - f"Reference panel bim file not found: {geno_path}.bim" - ) - if not geno_path.with_suffix(".fam").exists(): - raise FileNotFoundError( - f"Reference panel fam file not found: {geno_path}.fam" - ) - - # Validate labels file exists - if not labels_path.exists(): - raise FileNotFoundError( - f"Reference labels file not found: {labels_path}" - ) - - # Load genotype data - genotypes = GenotypeData.from_path(geno_path) - - # Load labels - labels_df = cls._load_labels_file(labels_path) - - # Load fam file to get sample IDs - fam_df = pd.read_csv( - geno_path.with_suffix(".fam"), - sep=r"\s+", - header=None, - names=["FID", "IID", "PAT", "MAT", "SEX", "PHENO"], - dtype=str, - ) - - # Merge labels with fam file - merged = fam_df.merge( - labels_df, - on=["FID", "IID"], - how="left", - ) - - # Check for missing labels - missing_labels = merged["label"].isna() - if missing_labels.any(): - n_missing = missing_labels.sum() - if require_all_labeled: - raise AncestryError( - f"{n_missing} samples in reference panel have no labels. " - f"Set require_all_labeled=False to allow unlabeled samples." - ) - # Filter to only labeled samples - merged = merged[~missing_labels] - - # Create Series with IID as index - labels_series = pd.Series( - merged["label"].values, - index=merged["IID"].values, - name="label", - ) - - return cls( - genotypes=genotypes, - labels=labels_series, - labels_path=labels_path, - ) - - @staticmethod - def _load_labels_file(path: Path) -> pd.DataFrame: - """Load ancestry labels file. - - Handles both headerless files and files with FID/IID/label header. - - Args: - path: Path to labels file. - - Returns: - DataFrame with FID, IID, label columns. - """ - # Try reading with assumed columns - df = pd.read_csv( - path, - sep="\t", - header=None, - names=["FID", "IID", "label"], - dtype=str, - ) - - # Check if first row looks like a header - if df.iloc[0]["FID"] in ("FID", "#FID"): - # Re-read with header - df = pd.read_csv(path, sep="\t", dtype=str) - # Normalize column names - df.columns = [c.replace("#", "") for c in df.columns] - if "label" not in df.columns: - # Try common alternatives - for alt in ("LABEL", "ancestry", "ANCESTRY", "pop", "POP"): - if alt in df.columns: - df = df.rename(columns={alt: "label"}) - break - - return df[["FID", "IID", "label"]] - - @property - def sample_count(self) -> int: - """Number of labeled samples in reference panel.""" - return len(self.labels) - - @property - def variant_count(self) -> int: - """Number of variants in reference panel.""" - return self.genotypes.variant_count - - @property - def ancestry_counts(self) -> Dict[str, int]: - """Count of samples per ancestry label. - - Returns: - Dictionary mapping ancestry labels to sample counts. - """ - counts = self.labels.value_counts() - return dict(counts) - - @property - def unique_ancestries(self) -> List[str]: - """List of unique ancestry labels in reference panel.""" - return list(self.labels.unique()) - - def get_samples_by_ancestry(self, ancestry: str) -> List[str]: - """Get sample IDs for a specific ancestry. - - Args: - ancestry: Ancestry label to filter by. - - Returns: - List of sample IIDs with the specified ancestry. - """ - return list(self.labels[self.labels == ancestry].index) - - def validate_snp_overlap( - self, - other: GenotypeData, - min_overlap: int = 10000, - ) -> int: - """Check SNP overlap with another genotype dataset. - - Ancestry prediction requires sufficient SNP overlap between - the reference panel and the target samples. This method - estimates the overlap (actual overlap is computed during - data preparation). - - Args: - other: GenotypeData to check overlap with. - min_overlap: Minimum required SNP overlap. Default is 10000. - - Returns: - Estimated number of overlapping SNPs. - - Raises: - AncestryError: If overlap is below minimum threshold. - """ - # This is a simplified check - actual overlap computation - # happens during data preparation when SNP IDs are compared - min_variants = min(self.variant_count, other.variant_count) - - if min_variants < min_overlap: - raise AncestryError( - f"Insufficient variant count for ancestry prediction. " - f"Reference has {self.variant_count} variants, " - f"target has {other.variant_count} variants. " - f"Expected at least {min_overlap} overlapping SNPs." - ) - - return min_variants - - def get_labeled_data(self) -> pd.DataFrame: - """Get labeled sample information. - - Returns: - DataFrame with FID, IID, and label columns for all - labeled samples in the reference panel. - """ - # Load fam file for FID/IID pairs - fam_df = pd.read_csv( - self.genotypes.path.with_suffix(".fam"), - sep=r"\s+", - header=None, - names=["FID", "IID", "PAT", "MAT", "SEX", "PHENO"], - dtype=str, - ) - - # Filter to labeled samples and add labels - labeled = fam_df[fam_df["IID"].isin(self.labels.index)].copy() - labeled["label"] = labeled["IID"].map(self.labels) - - return labeled[["FID", "IID", "label"]] - - -def get_default_model_path() -> Optional[Path]: - """Get path to bundled NBA model if available. - - Returns the path to the pre-trained NBA v2 model if it has been - downloaded using genotools-download. - - Returns: - Path to model directory, or None if not available. - """ - import os - - model_destination = os.path.expanduser("~/.genotools/ref") - nba_model_dir = Path(model_destination) / "models" / "nba_v2" - - if nba_model_dir.exists(): - return nba_model_dir - - return None - - -def validate_model_files(model_dir: Path) -> Dict[str, Path]: - """Validate that required model files exist. - - Checks for the presence of all files needed to load a trained - ancestry model. - - Args: - model_dir: Directory containing model files. - - Returns: - Dictionary mapping file types to their paths. - - Raises: - FileNotFoundError: If required files are missing. - """ - required_files = { - "model": "pipeline.pkl", - "common_snps": "common_snps.txt", - } - - found_files: Dict[str, Path] = {} - - for file_type, pattern in required_files.items(): - matches = list(model_dir.glob(pattern)) - if not matches: - raise FileNotFoundError( - f"Required {file_type} file not found in {model_dir}. " - f"Expected file matching pattern: {pattern}" - ) - found_files[file_type] = matches[0] - - return found_files diff --git a/genotools/cli/runner.py b/genotools/cli/runner.py index 1c98eb9..180581b 100644 --- a/genotools/cli/runner.py +++ b/genotools/cli/runner.py @@ -218,7 +218,7 @@ def _initialize_modules(self) -> None: ) # Import Ancestry module - from ..ancestry import AncestryModel, ReferencePanel, AncestryConfig + from ..ancestry import AncestryModel, AncestryConfig # Store module references self._new_modules = { @@ -235,7 +235,6 @@ def _initialize_modules(self) -> None: "run_gwas_association": run_gwas_association, "GenotypeData": GenotypeData, "AncestryModel": AncestryModel, - "ReferencePanel": ReferencePanel, } # Store config classes