feat: per-sample reference support via manifest ref/ref_name columns - #71
Merged
Conversation
The manifest can now carry ref and ref_name columns so each sample can use its own reference; the two-column manifest with config-level values keeps working, and a '.' cell falls back to the config value. Chromosome names and lengths come only from each sample's BAM header, in header order (the sort order of the data), and a generated per-sample genome file replaces the fai for all bedtools and bigtools sizes arguments (with a parse-time staleness guard, so a leftover genome file can never disagree with its bam header). All reference and chromosome lookups are functions of the sm wildcard. Includes the fixes from the multi-agent code review (manifest cells parsed as literal text, config-specific errors for null ref values, sample-attributed test assertions). Supersedes #44. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…enome file Every multi-chromosome bedtools sort now uses -g with the per-sample genome file (exclude_from_shuffle, clustering_vs_null, wide and one-percent peaks, hap-differences track), and the two sorted intersects in fires_in_peaks that ran without -g are anchored too — an unanchored -sorted sweep only worked while both inputs happened to share an order. exclude_from_shuffle also drops exclude-file contigs that are absent from the sample's bam header, since bedtools sort -g rejects undeclared chromosomes and those records were dead weight. Per-chromosome rules keep plain position sorts; chromosome order cannot apply within one chromosome. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
verify-multi now asserts full content equality between the test and test-rev samples (same reads, reversed header) for peaks and pileup — the one check that catches a silent ordering regression. The manifest reader uses index_col=False with ParserWarning promoted to an error, so a uniformly extra column fails loudly instead of column-shifting or silently dropping a field; the malformed-row check covers NaN and empty cells. config/README.md documents that outputs follow bam header order (not the lexicographic order of earlier FIRE versions) and that old results directories must be rerun from scratch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The content-equality guard checks file existence up front, so missing files fail loudly instead of comparing two empty streams. The manifest parse error gains a plain-language hint, the README notes that manifest paths cannot contain spaces or quotes (the whitespace-separated format cannot express them), and the err-nan-cell fixture description matches the validator that actually fires. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deleting files during parse runs on every invocation including dry runs, and a concurrent parse can remove a genome file a running job is reading. The staleness it guarded against (a bam swapped mid-run) also leaves every already-computed output stale, so regenerating only the genome file gives false confidence — swapped inputs require a fresh run, the same ancient() contract as the rest of the pipeline. The genome_file rule comment documents the accepted limitation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds per-sample reference genome support by extending the manifest format (ref/ref_name) and switching chromosome/contig handling to be derived from each sample’s BAM header (including a generated per-sample genome/chrom.sizes file used across bedtools/bigtools calls).
Changes:
- Extend manifest parsing/validation to support per-sample
refandref_name(with.sentinel fallback to config values). - Generate and consume a per-sample
genomefile (from BAM header) and anchor sorted bedtools operations using-g. - Add/extend test automation (dry-run matrix, regression verifiers, multi-sample fixture generation) and wire it into pixi tasks and CI.
Reviewed changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow/Snakefile | Removes global reference vars and updates wildcard constraints to be per-manifest/per-sample aware. |
| workflow/rules/common.smk | Implements manifest parsing/validation and per-sample ref/chrom/excludes accessors (BAM-header contigs). |
| workflow/rules/coverages.smk | Adds genome_file rule and updates coverage-related rules to use per-sample genome sizes and per-sample references. |
| workflow/rules/apply-model.smk | Updates fire/fire_sites to use per-sample references and per-sample chromosome expansion. |
| workflow/rules/fire-peaks.smk | Updates shuffling/pileup/peaks rules to use per-sample chrom lists and genome sizes for sorted ops. |
| workflow/rules/stats.smk | Updates stats rules to use per-sample genome sizes and anchors sorted intersects. |
| workflow/rules/decorated-reads.smk | Switches bigBed conversions to use per-sample genome sizes and per-sample chromosome expansion. |
| workflow/rules/track-hub.smk | Updates trackhub-producing rules to use per-sample genome sizes and per-sample ref_name. |
| workflow/envs/runner.yaml | Ensures runner env includes pysam (mirrors pixi env dependency). |
| tests/verify-results.sh | Adds regression golden-count verification script for single-sample test outputs. |
| tests/verify-multi.sh | Adds multi-sample verification script (per-sample genomes + order invariance checks). |
| tests/README.md | Documents test fixtures, pixi task entry points, and validator coverage matrix. |
| tests/generate-test-bams.py | Generates specialized BAMs for multi-sample testing (subset contigs + reversed header). |
| tests/dry-run-checks.sh | Adds a dry-run validation matrix for manifest/ref handling and rendered-shell assertions. |
| tests/config/two-sample.yaml | Adds a two-sample config exercising per-sample ref_name excludes behavior. |
| tests/config/two-sample.tbl | Adds a two-sample 4-column manifest fixture. |
| tests/config/single-4col.yaml | Adds a single-sample config for 4-column manifest mode. |
| tests/config/single-4col.tbl | Adds a single-sample 4-column manifest fixture. |
| tests/config/sentinel.yaml | Adds config fixture for . sentinel fallback behavior. |
| tests/config/sentinel.tbl | Adds manifest fixture using . sentinel in ref/ref_name. |
| tests/config/override-info.yaml | Adds config fixture asserting override INFO message behavior. |
| tests/config/no-fai.fa | Adds minimal fasta fixture to test missing .fai validation. |
| tests/config/no-excludes.yaml | Adds fixture ensuring “no excludes” branch renders correctly. |
| tests/config/multi.yaml | Adds multi-sample config for local-only multi-sample pipeline run. |
| tests/config/multi.tbl | Adds 4-sample manifest including generated BAM fixtures and sentinel usage. |
| tests/config/extra-exclude.bed | Adds small exclude BED used by dry-run/rendered-shell checks. |
| tests/config/err-sentinel-no-config.yaml | Adds negative fixture: sentinel requires config fallback value. |
| tests/config/err-sentinel-no-config.tbl | Adds negative manifest fixture for sentinel-without-config. |
| tests/config/err-ref-col-only.yaml | Adds negative fixture: manifest has ref but missing ref_name. |
| tests/config/err-ref-col-only.tbl | Adds negative manifest fixture for missing ref_name column. |
| tests/config/err-no-ref-anywhere.yaml | Adds negative fixture: no reference provided in config or manifest. |
| tests/config/err-nan-cell.yaml | Adds negative fixture: malformed short row in manifest. |
| tests/config/err-nan-cell.tbl | Adds negative manifest fixture for missing trailing fields. |
| tests/config/err-missing-refpath.yaml | Adds negative fixture: reference fasta path missing. |
| tests/config/err-missing-refpath.tbl | Adds negative manifest fixture with nonexistent ref. |
| tests/config/err-missing-fai.yaml | Adds negative fixture: missing .fai for ref. |
| tests/config/err-missing-fai.tbl | Adds negative manifest fixture referencing a fasta without .fai. |
| tests/config/err-missing-bam.yaml | Adds negative fixture: missing input BAM/CRAM path. |
| tests/config/err-missing-bam.tbl | Adds negative manifest fixture referencing nonexistent BAM/CRAM. |
| tests/config/err-keepchrs.yaml | Adds negative fixture: chromosome filters remove all contigs. |
| tests/config/err-extra-column.yaml | Adds negative fixture: extra column causes manifest parse failure. |
| tests/config/err-extra-column.tbl | Adds negative manifest fixture with an extra field. |
| tests/config/err-dup-sample.yaml | Adds negative fixture: duplicate sample names. |
| tests/config/err-dup-sample.tbl | Adds negative manifest fixture with duplicated sample IDs. |
| tests/config/err-config-ref-only.yaml | Adds negative fixture: config has ref without ref_name. |
| pixi.toml | Adds pysam dependency and expands test tasks (dry-run matrix + verify steps). |
| pixi.lock | Updates lockfile to include new deps and refreshed package resolutions. |
| CONTRIBUTING.md | Updates contributor guidance to run new validation and verify tasks. |
| config/README.md | Documents 2-col vs 4-col manifests, sentinel behavior, and BAM-header contig semantics. |
| .github/workflows/main.yml | Runs test-dry and test-verify in CI in addition to the main test. |
Suppressed comments (1)
workflow/rules/fire-peaks.smk:348
bedtools sortis invoked without specifying an input (-i -). If bedtools requires-i, this pipeline stage will fail at runtime (the stream comes from stdin here).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| FAI = get_fai() | ||
| REF_NAME = config["ref_name"] | ||
| EXCLUDES = get_excludes() | ||
| # chromosome filtering; references are per-sample (see the manifest columns) |
| bedtools genomecov -bg -i {output.null} -g {input.genome} | sed 's/$/\\tNull/g' | ||
| ) \ | ||
| | bedtools sort \ | ||
| | bedtools sort -g {input.genome} \ |
| ) \ | ||
| | cut -f 1-3 \ | ||
| | bedtools sort \ | ||
| | bedtools sort -g {input.genome} \ |
| | cut -f 1-3 \ | ||
| | bedtools sort \ | ||
| | awk 'NR==FNR {{keep[$1] = 1; next}} keep[$1]' {input.genome} - \ | ||
| | bedtools sort -g {input.genome} \ |
Member
Author
|
Response to the Copilot review. All four comments are incorrect for this pipeline, so no changes are made.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sample,bam,ref,ref_name. Each sample can use its own reference genome.refandref_namefor all samples..cell uses the config value for that row. Filled manifest cells override the config values. FIRE prints one INFO line about the override.genome_filerule writes a per-sample chrom.sizes file from the bam header. All bedtools and bigtools size arguments use this file. Every multi-chromosome sort and sorted intersect is anchored with-g.{sm}wildcard. Validation runs at parse time withFIRE:errors.ref_nameinto itsgenomes.txt.Supersedes #44 and #68 (#68 was closed by a base-branch deletion and cannot reopen).
Behavior changes
bedtools intersect -sortedcalls with-g. Rerun old results directories from scratch.Testing
pixi run test-dry: 27 fast dry-run checks for validation and shell rendering. Runs first in CI.pixi run testandtest-verify: the regression completes 49 of 49 rules with the same golden counts as main (1,181 peak lines, 46,223 elements).pixi run test-multi(local): four samples, 184 of 184 rules. Thetest-revsample has a reversed header. Its output content is identical to thetestsample, which proves order does not change results.