Refactor distance matrix calculation logic for consistency
Ensures that combined distance matrix calculations use the exact same site selection by retaining sampled data within Sankoff bundles. This involves refactoring post-sampling logic into shared functions, implementing `SankoffBundle` to reuse internal tally data, and adding validation to guarantee consistent site selection across all distance calculation paths.
This commit is contained in:
@@ -2674,6 +2674,80 @@ written in-house if the Guindon-Gascuel integration above is ever pursued
|
||||
their simulations, so plain NJ may be an acceptable substitute if BIONJ
|
||||
proper is judged not worth the effort).
|
||||
|
||||
### Sharing one sample across algorithms (`--sankoff`/`--tnt`/`--phyg`/`--iqtree` + `snp-*` `--distance`) — implemented 2026-09-11
|
||||
|
||||
**Correctness bug, not just a performance one.** `--sankoff`/`--tnt`/`--phyg`/
|
||||
`--iqtree` (via `SiblingExt::sankoff_bundle`) and a `snp-*` `--distance`
|
||||
(via `SiblingExt::snp_distance`) always consume the *exact same* selection
|
||||
parameters when requested together in one `obikmer phylo` invocation
|
||||
(`n`/`free_loss`/`no_ambiguity`/excluded-set/`entropy_bias` — the CLI has
|
||||
no way to give them different values in one run, see `cmd/phylo/args.rs`).
|
||||
Before this fix, each independently called `sample_index`, and
|
||||
`sample_layer`'s `rand::rng()` (`subsample.rs:235`) is a **thread-local
|
||||
generator that advances across calls, not reseeded each time** — so the
|
||||
second call silently drew a *different* random sample of sites than the
|
||||
first, even with identical parameters. Verified empirically (throwaway
|
||||
8-genome index): running `snp-jc` alone vs. combined with `--sankoff`
|
||||
produced visibly different distance matrices from the same index/params.
|
||||
This defeated the actual point of combining these flags — running several
|
||||
algorithms (Sankoff/TNT/PhyG/IQ-TREE calibration, a `snp-*` distance) on
|
||||
one *identical* site selection for direct comparison.
|
||||
|
||||
**Fix**: `SankoffBundle` (`sankoff.rs`) now retains its `PairwiseTally`/
|
||||
`PartitionDispersion` internally (previously consumed into `raw`/
|
||||
`base_pair_tally`/`cardinality_tally` and dropped) and exposes
|
||||
`SankoffBundle::snp_distance(kind, gamma_shape)`, which computes the
|
||||
matrix from that *same* already-sampled tally — no second `sample_index`
|
||||
call. The shared post-sampling logic (the `--gamma-shape` support check,
|
||||
`alpha` resolution, final matrix build — previously all inline in the
|
||||
`snp_distance()` free function) was factored into
|
||||
`pub(crate) fn distance_matrix(tally, dispersion, kind, gamma_shape)` in
|
||||
`snp_distance.rs`, called by both the standalone `snp_distance()` (after
|
||||
its own fresh sampling) and `SankoffBundle::snp_distance` (reusing the
|
||||
bundle's). `cmd/phylo/mod.rs` keeps the `Option<SankoffBundle>` alive past
|
||||
the Sankoff-family `if` block and, when a `snp-*` `--distance` is also
|
||||
requested, calls `bundle.snp_distance(...)` instead of
|
||||
`cache.snp_distance(...)` whenever a bundle was built — since both branches
|
||||
are driven by the same `args.*` fields, the parameters trivially always
|
||||
match when both fire; no runtime "do the params match" check needed.
|
||||
Verified end-to-end: the reused path logs `(reusing the Sankoff bundle's
|
||||
sample)` and the `snp_distance` stage timer reads `0ms` (`formula(kind)`
|
||||
is `O(n²)` post-processing, no I/O), vs. ~150-230ms for a fresh sample on
|
||||
the same tiny test index.
|
||||
|
||||
#### Future direction, not implemented: explicit `--session`
|
||||
|
||||
Raised in discussion, not started. The fix above only covers reuse
|
||||
*within one process*. A further idea: a `--session DIR` flag naming a
|
||||
directory (outside the index) that persists the CLI selection parameters
|
||||
plus every intermediate artifact `sample_index` would otherwise
|
||||
recompute — the site set itself, `PairwiseTally`/`PartitionDispersion`,
|
||||
the pseudo-alignment, etc. — across *separate* `obikmer phylo`
|
||||
invocations, e.g. running `--sankoff` today and `--distance snp-k2p`
|
||||
tomorrow against the identical sample. Without `--session`, an implicit
|
||||
*temporary* session would still be created (scoped to the index directory,
|
||||
matching the tmp-cache design discussed earlier in this file's α-estimation
|
||||
section) — same mechanism, just not named/kept by the user.
|
||||
|
||||
Two open design points if this is picked up:
|
||||
- **Cache-key validity**: as established for the tally cache above, the
|
||||
content depends on `n`/`free_loss`/`no_ambiguity`/excluded-set/
|
||||
`entropy_bias` — a session is only reusable for the exact tuple it was
|
||||
built under. `--subsample` in particular is the parameter most likely to
|
||||
change between exploratory runs, so an *exhaustive* (`n = None`) session
|
||||
that later runs subsample *from*, rather than one session per exact `n`,
|
||||
would likely see far more real reuse.
|
||||
- **Concurrency-safe cleanup**: `obisys::DirLock` (`lock.rs`) is the
|
||||
existing pattern to follow — an OS advisory lock (`flock`/`LockFileEx`),
|
||||
auto-released on process exit *including a crash*, no stale-lock cleanup
|
||||
logic needed. Applied per cache/session entry (not just once for the
|
||||
whole index as `DirLock` does today for annex writes): a process
|
||||
deciding whether to reclaim an old temporary session first tries to
|
||||
acquire that entry's lock — success means nobody's using it, safe to
|
||||
delete; failure means another process holds it, leave it alone. Avoids
|
||||
the hazard of one process deleting another concurrently-running
|
||||
process's temp session, which a naive "wipe at startup" would risk.
|
||||
|
||||
### Output format: PHYLIP-relaxed by default for the distance matrix
|
||||
|
||||
**Implemented.** The primary distance-matrix output
|
||||
|
||||
Reference in New Issue
Block a user