fix(depth): 0-based output is half-open, not closed (#427)

[mwiewior]

pb.depth(..., use_zero_based=True) returned 0-based closed blocks instead of the documented half-open. Both coordinates were shifted down by one while the interval stayed closed, so every block covered one base fewer than its 1-based counterpart — on tests/data/io/cram/test.cram, summing block widths gave 36 bases instead of 117.

The defect is upstream: PileupConfig::zero_based reached the BAM/CRAM provider and the schema metadata but never the coverage-block emitters, which hardcoded an inclusive end. Fixed in
biodatageeks/datafusion-bio-functions#205; pin the pileup dep to that rev until it lands in a release.

Tests: the existing coordinate-system tests only asserted the schema metadata flag, and the samtools golden-parity suite ran exclusively with use_zero_based=False, so nothing covered the actual 0-based coordinates. Add value-level tests over BAM/SAM/CRAM (half-open ends, the equal-width invariant across systems, block chaining, first-block-at-zero) and extend the NA12878 10k golden suite with a 0-based parity test across 1/2/4 partitions.

All 12 new tests fail against the previous pileup pin and pass on the fix, while the 28 pre-existing pileup tests pass either way — which is precisely why this reached users. Against the golden dataset the old build recovers 19,888 of 21,155 covered positions.

Description

Related issue

Closes #

Type of change

• [ x] 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 💥 Breaking change (fix or feature that changes existing behavior)
• 📖 Documentation update
• 🧹 Refactor / chore / CI (no functional change)
• ⚡ Performance improvement

How has this been tested?

Checklist

• My code follows the style and conventions of this project.
• I have performed a self-review of my own code.
• I have added or updated tests that prove my fix is effective or my feature works.
• New and existing unit tests pass locally (python -m pytest) and, for Rust changes, cargo check / cargo test pass.
• I have run maturin develop --release and verified the change works end-to-end where applicable.
• I have updated the documentation (docstrings, docs/) as needed.
• I have updated CHANGELOG.md under [Unreleased] where user-facing behavior changed.
• My changes generate no new warnings.
• I have read and agree to the Code of Conduct.

Additional notes

[mwiewior]

@codex pls review

[mwiewior]

@claude pls review

[claude]

Claude finished @mwiewior's task in 2m 14s —— View job

---

Code Review

• Gather context
• Read changed files
• Review CHANGELOG.md
• Review Cargo.toml / Cargo.lock
• Review tests/test_pileup.py
• Review tests/test_pileup_samtools.py
• Synthesize findings and post review

---

Overall: ✅ Looks good — one medium concern, a few nits.

The fix is clean and minimal (no Python code changes — correct: the bug was upstream). The test suite is thorough and the failure message in the assertion is particularly good ("A short count means 0-based ends are inclusive rather than exclusive" at test_pileup_samtools.py:180). CHANGELOG entry is well written.

---

Medium: Dependency jumps 3 minor versions

Cargo.lock shows datafusion-bio-function-pileup moving from 0.11.0 → 0.14.0. The PR description says "pin to the #205 fix commit", but the Cargo.toml at that commit declares version 0.14.0, which means everything released between 0.11 and 0.14 is being pulled in, not just the coordinate fix.

# Cargo.toml:66
datafusion-bio-function-pileup = { ..., rev = "d8f48e07bf90faad54f4470433119a7d7f44af84", ... }

This is fine if the 0.12/0.13/0.14 pileup releases are safe, but worth a quick scan of the upstream changelog. The datafusion-bio-function-ranges crate (from the same monorepo) stays at v0.11.0 — those two crates sharing a datafusion = "=53.0.0" pin is the main compat concern.

Suggestion: Confirm the 0.12–0.14 intermediate pileup releases don't include any breaking changes or regressions vs. the current build; add a tracking issue or TODO pointing to the upstream release to make it easy to retag later.

---

Low: Row-order assumption in test_depth_zero_based_blocks_are_half_open

test_pileup.py:106-114:

one_based = pb.depth(path, use_zero_based=False).collect()
zero_based = pb.depth(path, use_zero_based=True).collect()

assert (zero_based["pos_start"] == one_based["pos_start"] - 1).all()
assert (zero_based["pos_end"] == one_based["pos_end"]).all()

The element-wise comparison assumes the two collect() calls produce rows in the same order. With a single partition and a small file this is deterministic in practice, but pb.depth() makes no documented ordering guarantee. A .sort(["contig", "pos_start"]) on both DataFrames before comparison would make the test immune to future parallelism changes. Fix this →

---

Nit: test_depth_zero_based_blocks_chain_without_gaps_or_overlap runs only on CRAM

test_pileup.py:133 — the chain test and the first-block-at-zero test both hardcode CRAM_PATH. The equal-width test and the half-open test are already parametrized over all three formats (BAM_PATH, SAM_PATH, CRAM_PATH). Consistency would favour parametrizing the chain test too, but this is a very minor point given the samtools suite covers all three formats indirectly.

---

Nit: Cargo.toml comment mentions #205 only; CHANGELOG mentions both #204, #205

Cargo.toml:64 says biodatageeks/datafusion-bio-functions#205; CHANGELOG.md:14 says (biodatageeks/datafusion-bio-functions#204, #205). Minor inconsistency — worth aligning. Fix this →

---

What's well done

• The bug is precisely identified: PileupConfig::zero_based reached the schema but not the block emitters. The fix scope (single upstream dep pin) is surgical.
• _expand_blocks_zero_based correctly uses int_ranges(pos_start, pos_end) (exclusive end) vs _expand_blocks's int_ranges(pos_start, pos_end + 1) — this is the critical difference and it's right.
• The try/finally wrapping around set_option in _run_blocks_zero_based is consistent with the rest of the test helpers.
• The PR description's post-mortem ("the existing coordinate-system tests only asserted the schema metadata flag") is a useful reminder of why value-level tests matter.
• 12 new tests that all fail on the old pin and pass on the fix is exactly the right evidence to include in a PR description.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: d71061d920

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[mwiewior]

Repointed the pileup dependency from the temporary rev to tag v0.14.1, which is where biodatageeks/datafusion-bio-functions#205 landed — so the "switch back to a tag once that lands in a release" note in Cargo.toml is now settled.

No behaviour change: v0.14.1 is that fix (447dba4) plus the release commit. Cargo.lock resolves to datafusion-bio-function-pileup v0.14.1.

Full suite passes — 1080 passed, 3 skipped — including the value-level 0-based coordinate tests this branch adds.

I left ranges on v0.11.0 and fastqc on v0.13.1 rather than moving everything to v0.14.1, to keep this PR to the one dependency it is about. Worth knowing though: the three pins now resolve the same repository at three different refs, so cargo checks it out three times. Collapsing them onto a single tag would remove that duplication, but ranges v0.11.0 → v0.14.1 spans real changes, so it deserves its own PR rather than riding along here.