refactor(bed): extract classify(), add merge(), and drop the bed_exists guard (#231)

[TimD1-bot]

Note

Authorship: the content below was drafted by Claude Opus 5 (an AI coding agent) and
filed via gh under @TimD1-bot, a bot account operated by @TimD1. It reflects the
agent's analysis, not a statement authored by @TimD1.

Summary

Three edits to src/bed.cpp, all prerequisites of the #47 membership sweep, none of which
changes any output. Every TSV, the summary VCF, and every counter are what they were.

Change

Extract a pure classify()

contains() interleaved two concerns: locating the variant (start_idx via upper_bound
on starts, stop_idx via lower_bound on stops) and classifying it from how those
indices relate. classify() now takes the decision tree plus the before-all / after-all
BED_OUTSIDE early returns, which are load-bearing rather than incidental: a variant entirely
left of the first region yields start_idx == -1 and would otherwise be misclassified
BED_BORDER by the very next test. Only the BED_OFFCTG contig-presence check and the
stop < start ERROR stay with the caller.

contains() keeps both binary searches and delegates, so it returns exactly what it did before.
The two BED_OUTSIDE cases now run the searches before short-circuiting, which is two extra
log n probes on a variant that was going to be discarded.

Why this is not optional. #47 adds a second way of locating a variant — a monotonic cursor
instead of a binary search. If that cursor reimplemented INSIDE/BORDER/OUTSIDE alongside
contains(), there would be two implementations of one rule, free to drift, and the contains()
copy is the one the current tests pin. There is now one decision tree with two ways of reaching it.

Drop the g.bed_exists guard

contains() opened with if (!g.bed_exists) return BED_INSIDE; — a method on one bedData
consulting a global flag about a different bedData. For a stratification region set that is
outright wrong: membership would depend on whether -b was supplied. The check moves to the sole
-b call site in parse_variants():

bedloc_t loc = g.bed_exists ?
        g.bed.contains(ctg, rec->pos, rec->pos + reflen, type) : BED_INSIDE;

This also makes contains() unit-testable without global setup — the nine BedContains cases no
longer carry g.bed_exists = true scaffolding.

Add normalize() and a lenient constructor

bedData(const std::string & bed_fn, bool normalize = false);
void normalize();  // sorts by start, merges overlapping/adjacent intervals per contig

normalize() recomputes size from the merged intervals — bases covered by two input intervals
were counted twice by add() — warns when the regions were not already sorted, and at verbosity

= 2 reports how many intervals coalesced. Both messages name the BED file, since #47 normalizes
many region sets and a message about one of them has to say which; bedData carries a filename
member for that. Regions are sorted by start then stop so a nested interval follows the one
containing it, and the running stop is extended with max, so an enclosing region is not
truncated to a nested one's end.

check() still runs either way, after normalize() rather than instead of it. Normalizing
does not weaken the validation, because disorder and overlap are the only malformations it
repairs: a flipped or zero-length interval is left exactly as it was found and is still fatal.
-b passes normalize = false and so behaves exactly as it does on dev — strict validation of
an evaluation region whose malformation would silently change every denominator. Normalizing is
for the third-party region sets we neither author nor control.

Inert in this PR: no caller passes true yet. Normalizing is not tidiness — contains()'s
two binary searches require sorted, non-overlapping intervals, so an unnormalized region set
would return wrong answers rather than merely being untidy.

Testing

21 cases added, one retargeted, none of the existing contains() assertions changed.

test	covers
BedNormalize.AlreadyMergedUnchanged / SortsUnsorted	disjoint regions are ordered, never combined
BedNormalize.CombinesOverlapping / CombinesAdjacent	both coalesce, and size stops double-counting
BedNormalize.AbsorbsNested / CollapsesDuplicates	the enclosing stop survives; identical copies collapse to one
BedNormalize.SingleIntervalUnchanged / EmptyBedUnchanged	the two degenerate inputs
BedNormalize.MergesEachContigSeparately	no region merges across a contig boundary
BedNormalize.MergedRegionsAreQueryable	contains() agrees with the normalized layout, which is what it assumes
BedNormalize.WarnsWhenUnsorted	the unsorted warning fires, names the file, and the regions come back sorted
BedNormalize.ReportsCoalescedCount	the verbosity >= 2 report names the file — and merging alone does not report the file as unsorted
BedNormalize.SilentWhenAlreadyNormalized	a sorted, disjoint set is repaired in no respect, so neither message fires
BedCtor.NormalizedOverlapPassesCheck	the file BedCtor.RunsCheck rejects is accepted once normalized, with the check still running and stderr empty
BedCtor.NormalizeStillRunsCheck	a flipped and a zero-length region stay fatal with normalizing enabled
BedCtor.RecordsFilename	the filename the messages report is the one the constructor was given
BedClassify.AgreesWithContains	every start/stop pair in 0–50 over two regions, for SUB and INS, classifies the same through both entry points
BedClassify.BeforeAllOutsideNotBorder / AfterAllOutsideNotBorder	the load-bearing early returns, with the off-the-end index asserted explicitly
ParseVariants.NoBedEverythingInside	the retargeted BedContains.NoBedInside: with no -b, a variant is kept as BED_INSIDE even though g.bed holds regions that exclude it

The extraction's guard is that contains()'s existing return values are unchanged. To confirm the
tests can see a break, classify()'s early returns were reordered after the index tests: four cases
fail, two of them pre-existing (BedContains.BeforeAllOutside, BedContains.AfterAllOutside).

Verification

• Unit suite: 781 tests, all passing, from make clean in both build trees.
• pytest (integration + unit workflow): 124 passed. No integration case was added — nothing
  user-visible changed.
• Both builds compile clean under -Wall -Wextra -Werror=missing-field-initializers; doxygen src/Doxyfile emits no warnings.
• chr20 fixture A/B against dev: all eight count-bearing outputs byte-identical, zero variants
  reclassified.

Part of #47. Resolves #231.

[TimD1]

still check even if you just merged

[TimD1-bot]

Claude Opus 5 🤖: check() now runs unconditionally, after normalize() rather than instead of it. Normalizing repairs disorder and overlap and nothing else, so a flipped or zero-length interval is left as found and still errors — BedCtor.NormalizeStillRunsCheck pins both.

[TimD1]

rename merge_overlaps() to normalize() (sort and merge)

[TimD1-bot]

Claude Opus 5 🤖: Renamed the constructor parameter merge_overlaps to normalize, matching the method.

[TimD1]

please rename to "normalize()", since this function sorts and merges

[TimD1-bot]

Claude Opus 5 🤖: Renamed merge() to normalize(), since it sorts as well as merges.

[TimD1]

Can we also add a warning here if we discover the input wasn't sorted?

[TimD1-bot]

Claude Opus 5 🤖: Added an unconditional WARN when the regions were not already sorted, detected with std::is_sorted on the same key the sort uses. BedNormalize.WarnsWhenUnsorted covers it; ReportsCoalescedCount pins that merging alone does not report the file as unsorted.

[TimD1]

We should report the name of the BED file when displaying info.

[TimD1-bot]

Claude Opus 5 🤖: Both messages now name the BED file. bedData gained a filename member set by the file constructor, empty for an in-memory one.

[TimD1]

this will be a mis-nomer, since I still want the check, it should just pass once things have been normalized. and obviously rename "Merge" to "Normalize"

[TimD1-bot]

Claude Opus 5 🤖: Renamed to BedCtor.NormalizedOverlapPassesCheck and reframed: it now asserts the check still runs and passes, with stderr empty. Suite BedMerge renamed to BedNormalize throughout.

[TimD1-bot]

Claude Opus 5 🤖: All six comments addressed — merge() is now normalize(), check() runs after it rather than instead of it, and normalizing warns on unsorted input and names the BED file in both messages. No count impact: every count-bearing output is byte-identical on the chr20 fixtures.

Responses to review comments (6 addressed, 0 deferred)

• src/bed.cpp:97 — still check even if you just merged. check() now runs unconditionally, after normalize(). This does not weaken the validation: normalizing repairs disorder and overlap and nothing else, so a flipped or zero-length interval is left exactly as found and still errors. BedCtor.NormalizeStillRunsCheck pins both, and BedCtor.NormalizedOverlapPassesCheck pins that the check passes once the overlap is repaired.
• src/bed.cpp:96 — rename merge_overlaps. The constructor parameter is now normalize, matching the method it selects.
• src/bed.cpp:171 — rename merge() to normalize(). Done, along with its doc block.
• src/bed.cpp:203 — warn if the input wasn't sorted. Added an unconditional WARN, detected with std::is_sorted on the same (start, stop) key the sort uses, so what it reports and what it reorders cannot disagree. BedNormalize.WarnsWhenUnsorted covers it; ReportsCoalescedCount pins the converse — merging alone is not grounds for calling the file unsorted.
• src/bed.cpp:204 — report the BED filename. bedData gained a filename member, set by the file constructor and empty for an in-memory one; both the warning and the verbosity-2 info now name it. The message tests load from a file rather than building in memory, since the file being named is what they assert on.
• tests/unit/src/test_bed.cpp:119 — misnomer, and rename Merge to Normalize. MergeOverlapsSkipsCheck is now NormalizedOverlapPassesCheck and asserts the check runs and passes (stderr empty), rather than that it was skipped. The BedMerge suite is BedNormalize throughout.

Threads are replied to but left unresolved: enumerating thread node ids needs a GraphQL read that this environment denies, so resolving them is yours to do.

Count impact — no change on the chr20 fixtures

Tier: chr20 fixtures (tests/integration/data), chr20.bed. Base faf2ace, PR 796ff22.

All eight count-bearing outputs are byte-identical between base and PR: precision-recall-summary.tsv, precision-recall.tsv, query.tsv, truth.tsv, genotype-errors.tsv, phasing-summary.tsv, phase-blocks.tsv, switchflips.tsv. Zero variants changed classification. summary.vcf differs on one line of 72,605 — the recorded ##CL= command line, which carries the output prefix — and parameters.tsv likewise; runtime.tsv holds timings.

This is expected rather than lucky: -b passes normalize = false, so the evaluation path runs the same check() it always did, and the classify() extraction preserves contains()'s returns by construction.

Verification

• Unit suite: 781 tests, all passing, from make clean in both src/ and tests/unit/build/.
• pytest: 124 passed.
• No warnings under -Wall -Wextra -Werror=missing-field-initializers.

Rebased twice during this run, both clean and conflict-free — dev moved from 62b7859 to 6780c48 to faf2ace while the work was in flight, so the force-push rewrites only my own two commits onto the current tip.

One thing worth knowing, since it cost a false test run: adding a member to bedData changes sizeof(Globals), and the hand-maintained header dependencies in both Makefiles do not list bed.h for objects that reach it transitively through globals.h. An incremental build produced 20+ bogus failures in cluster/supercluster tests; make clean cleared them. Not this PR's to fix, but a follow-up issue on those dependency lists would be worth filing.