Skip to content

fix(merge): rebuild signal table at a uniform batch stride - #432

Merged
jayhesselberth merged 2 commits into
mainfrom
fix-merge-batch-stride
Oct 5, 2026
Merged

jayhesselberth merged 2 commits into
mainfrom
fix-merge-batch-stride

Conversation

@jayhesselberth

Copy link
Copy Markdown
Member

Why

merge concatenated each input file's own Arrow signal batches as-is, offsetting only byte positions. Almost every POD5 file's own last batch is short (read count rarely divides evenly by batch size), so merging N files put a short batch from file k immediately before a full one from file k+1 at every file boundary but the last — breaking the constant stride dorado and the official pod5 library assume between batches (escapepod's own reader walks real cumulative row counts and is unaffected — same bug class as #195, writer-side).

Reproduced on real production data: merging 20 real multi-file flexizyme POD5 directories and basecalling the output with dorado 2.1 gave Failed to get read signal - 'Invalid: Too few samples in input samples array' on every one of them. escpod inspect summary independently confirms: Signal batches: NOT PORTABLE — signal batch 46 has 31 rows, expected 100.

What

merge now flattens every surviving (non-duplicate) read's compressed signal chunks across all inputs and rebuilds the signal table at one uniform stride via write_raw_signal_table — the same correctness-first approach filter/subset already used. That writer moved from operations::filter into utils::table_builders, next to the SignalRow/build_signal_batch primitives it's built from, since it's now shared by three callers instead of one (layering fix, not just the bug fix). Compressed bytes are still copied without decompression/recompression — only the Arrow batch grouping is rebuilt. MergeOptions gains signal_batch_size (default 1_000, matching FilterOptions).

Side effect, not the point of the change: a duplicate read's signal bytes are no longer carried into the output, since extraction now happens per-surviving-read instead of per-input-file (previously every input file's full signal table was copied regardless of later dedup).

Testing

  • New regression test (merge_output_signal_batches_stay_uniform_even_with_short_trailing_inputs): merges two 150-read fixtures (each batches as [100, 50] under default WriterOptions) and asserts reader.nonuniform_signal_batch().is_none() on the output — this is exactly the shape that broke before the fix.
  • cargo nextest run -p escapepod-pod5: 257/257 pass, including the existing merge_preserves_signal_bytewise (no recompression) and the full test_read_batch_geometry/test_merge_integration suites.
  • cargo clippy -p escapepod-pod5 --all-targets -- -D warnings: clean.
  • cargo build -p escapepod-cli: builds against the new MergeOptions field.
  • Real-data validation: rebuilt the 16-file, 59,418-read lys flexizyme merge with this branch's release binary, then compared every read's signal array between the merged output and the original raw files using the official pod5 Python library (independent of this crate): zero mismatches, zero missing, across all 59,418 reads. escpod inspect summary no longer reports NOT PORTABLE on the new output (did on the old one, reproduced above).
  • dorado 2.1 --emit-moves basecalling of both the raw directory and the fixed merged file no longer errors on either. (Basecalled read yield between the two differs — see follow-up note below; this is a separate, pre-existing issue, not a data-correctness regression from this change.)

Follow-up (not in this PR)

While validating, basecalling the full lys corpus via dorado 2.1 yielded 59,411/59,418 reads from the raw directory but only 34,186/59,418 from the merged file — despite the signal being byte-identical (proven above). The likely cause: collect_pod5_inputs (escapepod-cli/src/util.rs) sorts input files with plain Vec::sort(), which is lexicographic over filenames like ..._0.pod5, ..._1.pod5, ..._10.pod5, ... — scrambling the numeric/chronological file order MinKNOW wrote. If dorado's read-splitter relies on channel/time continuity across adjacent reads in the stream, concatenating files out of chronological order would explain a large, order-sensitive split-rate change with identical bytes. Worth a natural-sort fix and its own dorado-based verification as a separate change.

merge concatenated each input's own Arrow signal batches as-is, offsetting
only byte positions. Almost every POD5's own last batch is short, so
merging N files put a short batch from file k before a full one from file
k+1 at every file boundary but the last. escapepod's own reader walks real
cumulative row counts and is unaffected, but dorado and the official pod5
library assume a constant stride and mis-resolve every read after the
break -- reproduced with dorado 2.1 ("Too few samples in input samples
array") on real multi-file POD5 merges.

merge now flattens every surviving read's compressed signal chunks across
inputs and rebuilds the table at one uniform stride via
write_raw_signal_table, moved from operations::filter into
utils::table_builders (alongside the SignalRow/build_signal_batch
primitives it's built on) and now shared by filter/subset and merge alike.
Compressed bytes are still copied without decompression/recompression.

Verified on a real 16-file, 59,418-read merge: zero signal mismatches
against the pod5 library (ground truth, independent of this crate), and
`escpod inspect summary` no longer reports the batch as NOT PORTABLE.
Pre-existing on main (unrelated to the merge fix); fixing here since it
blocks this PR's CI.
@jayhesselberth
jayhesselberth merged commit abe8ea4 into main Oct 5, 2026
9 checks passed
@jayhesselberth
jayhesselberth deleted the fix-merge-batch-stride branch October 5, 2026 14:57
jayhesselberth added a commit that referenced this pull request Oct 5, 2026
…ilders (#434)

## Why

Follow-up to #432's merge batch-stride fix: an architecture review
afterward
surveyed the rest of the workspace for the same cross-module layering
smell
(a function stranded in one module that a sibling reaches into, or
near-duplicate logic copy-pasted across modules). Two findings, filed as
#433.

## What

- Delete `utils/run_info.rs` — `add_run_infos_deduplicated`/
`map_run_info_index` were never wired into `utils/mod.rs` and have zero
call sites; superseded by `pod5_assembler::deduplicate_run_infos`, which
  `merge`/`filter` already share.
- Unify `build_reads_table` (merge, owned `(ReadData, Vec<u64>)` pairs)
and
`build_reads_table_remapped` (filter, borrowed `FlatReadRef`) behind one
  generic `build_reads_table_generic<R: PartitionRow + Sync>`. Both were
  ~200-line copies of the same dictionary-collection/partition/concat/
batch-write skeleton, differing only in input shape — already abstracted
  at the per-row level by the existing `PartitionRow` trait and
  `build_partition_inner`. Both public names stay as thin wrappers so
  callers in `merge.rs`/`operations/filter.rs` are untouched.

No behavior or output change — pure extraction, verified by the existing
`test_merge_integration.rs` / filter / subset test suite (unchanged, all
passing) plus a full `cargo test -p escapepod-pod5` run.

## Testing

- `cargo test -p escapepod-pod5`: all green (incl. doctests)
- `cargo fmt --all --check`: clean
- `RUSTFLAGS=-Dwarnings cargo clippy --workspace --all-targets`: clean

Closes #433
@jayhesselberth jayhesselberth mentioned this pull request Oct 6, 2026
jayhesselberth added a commit that referenced this pull request Oct 6, 2026
Patch release.

- Fixed: `merge` no longer produces non-portable output when inputs
don't share one batch stride (#432)
- Internal: dead run_info dedup removed, reads-table builders unified
(#434)

Version bump in root Cargo.toml (workspace + 5 path deps), CHANGELOG
rolled, Cargo.lock regenerated; `cargo check --workspace` clean.

Tag `v0.31.1` on the merge commit after merge (publishes to GitHub
Releases and PyPI).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant