Anchor the expected-MI recurrence at the mode; guard expected_mi() and quartet_concordance() inputs - #127
Merged
Merged
Conversation
Anchor the expected-MI recurrence at the mode, and guard its inputs expected_mi() seeded its recurrence over the hypergeometric distribution of cell overlaps at the smallest overlap the marginals allow. That probability is around 2^-1197 for a balanced split of 1200 tips, so it underflowed to zero, and the recurrence being multiplicative, every later term stayed zero: the function returned exactly 0, and ClusteringConcordance(normalize = TRUE) silently reported uncorrected mutual information. The recurrence now starts at the mode, whose probability is the largest of at most N + 1 values summing to one and so is always representable, and walks outwards in both directions. Also: reject an ni that is not a pair, matching mi_key(); guard the log-factorial table against a negative index, which an out-of-range ni could reach; replace the GCC/Clang constructor attribute with a block-scope static, whose initialization C++17 makes thread-safe; and reject negative state codes in quartet_concordance(), which index its count buffers directly. Fixes #91 Fixes #104 Fixes #105 Fixes #107 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> @
Sharpen the expected-MI tests and NEWS after review The NA case in the quartet block compared two structurally all-zero results, so it could not show the new non-negative guard leaves NA alone; it now uses six taxa, where dropping one would change the count. Assert the counts themselves, not just their shape. Cover the multi-state `nj` the real caller passes, and `.ExpectedMI()`, the memoised entry point through which ClusteringConcordance() reaches this arithmetic. NEWS: a wider sweep (600 partitions, N up to 3000) puts the change to already-correct values at 1.5e-11 rather than 2.3e-12, and shows the defect truncates the sum silently rather than only zeroing it -- as little as a quarter of the true value survived where some blocks underflowed and others did not. The threshold is 1080 tips scored for a character, not tips in the tree. Note the quartet guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> @
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.
Fixes #91
Fixes #104
Fixes #105
Fixes #107
Four defects in
src/expected_mi.cppandsrc/quartet_concordance.cpp. Noexported signature changes:
Rcpp::compileAttributes()on this tree returnssrc/RcppExports.cppandR/RcppExports.Rbyte-identical, sosrc/ts_rcpp.cpp,src/TreeSearch-init.cand the RcppExports pair are untouched and this branchdoes not collide with #84, #122 or #56.
#91 — the underflow, and why anchoring at the mode fixes it
expected_mi()sums, over each blockmjof the second partition, the mutualinformation contributed by an overlap
kwith the first partition's blocka,weighted by the hypergeometric probability
The old code evaluated
P(K = kmin)once by exponentiating a sum of logfactorials, then walked upwards with the exact ratio
P(k+1)/P(k) = (mj - k)(a - k) / ((k + 1)(N - mj - a + k + 1)).The seed is the problem.
kmin = max(0, a + mj - N), so for a near-balancedsplit against a near-balanced character
kmin = 0andP(K = 0) = 1 / C(N, a),which is about
2^-1197atN = 1200. That is below the smallest subnormaldouble (
2^-1074), so it becomes exactly zero; the recurrence ismultiplicative, so every subsequent term is zero too, and the
if (Pk > 0.0)guard suppresses that block's whole contribution.
The symptom is worse than "returns 0". The seed is evaluated per block
mj,so a character whose blocks straddle the threshold has some blocks silently
dropped from the sum and others counted, and
expected_mi()returns aplausible-looking but truncated number. Over 600 random partitions with N from
10 to 3000 (below), 60 came back wrong; of those only 4 were exactly zero, and
the rest ranged from a part in a million down to 26 % of the true value.
ClusteringConcordance(normalize = TRUE)subtracted whatever survived.The fix keeps the same recurrence but seeds it at the mode
kmode = floor((mj + 1)(a + 1) / (N + 2)), clamped to[kmin, kmax], and walksoutwards in both directions (the downward step uses the reciprocal ratio,
P(k-1)/P(k) = k(N - mj - a + k) / ((mj - k + 1)(a - k + 1))).P(K = kmode)is the largest of at most
N + 1probabilities that sum to one, so it is atleast
1/(N + 1)and can never underflow — the seed is now unconditionallyrepresentable rather than accidentally so. The total number of
kvaluesvisited is unchanged, so the per-
kcost the recurrence was introduced for(commit 7904371, 2025-09-01) is preserved in full. Because the distribution is
log-concave and
kmodeis an argmax, once a tail term reaches exactly zeroevery remaining term in that direction is also exactly zero under the same
multiplicative recurrence, so each sweep breaks there; the break is exactly
equivalent to running on, not merely a good approximation.
One incidental hazard goes with it: the ratio was formed as
static_cast<double>((mj - k) * (a - k)), i.e. the product was taken inintand overflows above roughly
a·mj > 2^31; it is now formed indouble.Evidence
Two reference implementations:
ReferenceEmi()computes the same sum from R'slchoose(), which works in log space and cannot underflow at the tails, and asecond check recomputes it from
dhyper(). Both are independent implementationsbut share the closed form, so they test the recurrence's numerics, not the
formula. The formula itself is checked separately, by the asymptote below and by
a formula-free permutation estimate.
Correctness above the old ceiling. For two balanced binary partitions the
expected mutual information has the closed-form asymptote
1/(2N ln 2)bits(
2N·MIin nats converges on a 1-df chi-square, whose mean is 1), which thecomputed values approach as
1 + 1.5/N:expected_mi1/(2N ln 2)This is the independent test of the form, and it is the strongest evidence
here: the fixed values are not merely non-zero, they land on an analytic
prediction derived without reference to the code. They are also strictly
decreasing in N across 100…5000, as a chance correction must be.
Agreement with the log-space oracle.
Nin 2…40, everya, everymj):worst relative deviation 2.8e-13. A
kcounted twice or missed would show uphere immediately.
Nin {10, 30, 100, 300, 700, 1000, 1200, 1500, 2000,3000},
njof 2–6 blocks: worst relative deviation 3.6e-11 (the largest atN = 3000, where the accumulated error of the 8192-entry log-factorial table
dominates).
Nuniform on 4…900: worst 4.7e-12.Agreement with the pre-fix values where the pre-fix code was right. Pre-fix
values come from
origin/cpp-search's sources compiled unchanged into aseparate library. Of the 600 random partitions, 540 were accurate pre-fix
(within 1e-10 of the oracle); on those the fixed value differs from the pre-fix
value by at most 1.5e-11 relative, and the two bracket the oracle rather
than one being systematically nearer it (worst |pre-fix − oracle| 2.8e-11,
worst |fixed − oracle| 2.0e-11, neither consistently smaller). Where the old
code was right, the new code is right to the same order — this is not a change
of answers, it is the same recurrence started somewhere it can be started.
These are sample maxima, not proven bounds; an independent replication by a
reviewer at N ≤ 900 measured 2.3e-12 against my 2.2e-12 on the same regime.
Worked examples:
Who was affected. The relevant
Nis the number of tips a characterscores, not tips in the tree (
R/Concordance.R:216,tabulate()over non-NAstates), so heavy missing data lowers the exposure. Against a perfectly even
character the first spurious zero appears at N = 1080; sweeping every split
size
a:aThe band is centred on the balanced splits — the deep edges of a large tree,
where the correction matters most — and it widens with N. Against uneven
characters the truncation is partial rather than total, so the count of exact
zeros understates the reach: on the 600-partition sample, 1 of 60 partitions was
wrong at N = 1200, 13 of 60 at 1500, 30 of 60 at 3000.
#104 —
ni[1]read out of boundsexpected_mi()now stops with "ni must be a vector of length 2." whenni.size() != 2, matching the guardmi_key()already had and the contractman/expected_mi.Rdhas always documented ("Integer vector of length two").This is not theoretical: on the pre-fix build,
expected_mi(integer(0), c(2L, 5L))reads two garbage
ints and then indexes the log-factorial table with them —at
-O2 -DNDEBUG, the pre-fix test run segfaults at that line (exit 139).#104 and #105 compose into a crash; either guard alone stops it.
#105 — portability and the unguarded table index
__attribute__((constructor))is a GCC/Clang extension. It is replaced by ablock-scope
static const std::vector<double>initialised by a lambda on firstuse.
DESCRIPTIONdeclaresSystemRequirements: C++17, and C++11 onwardsguarantee that such an initialisation runs exactly once even if several threads
enter concurrently, the others blocking until it completes — so this is not a
thread-safety regression relative to the constructor attribute, which ran before
any thread existed. The table is read-only afterwards, so concurrent
expected_mi()calls remain safe. (No caller runs it concurrently today; theguarantee matters only if one later does.)
l2factorial()now rejects anegative argument rather than indexing the table below zero.
#107 — negative state codes
quartet_concordance()uses each non-NAstate code directly as an index intoits
n0/n1count buffers. It now stops with "charactersmust containnon-negative state codes." rather than trusting the R-side convention. The
guard sits after the existing
is_natest, soNA_integer_(INT_MIN) isstill treated as a missing entry, not a negative code. Pre-fix, a
-1in thematrix silently returned all-zero counts (an out-of-bounds read/write,
undefined behaviour that happened not to fault here).
Tests
New file
tests/testthat/test-expected-mi.R, 28 assertions, Tier 1 (arithmeticonly, whole file under a second). Verified against a build of
origin/cpp-search's unmodified sources:.ExpectedMI(); not all positive; not decreasingnimust be a pair (#104)integer(0)then segfaults the sessionTwo caveats stated plainly rather than papered over. First, the small-partition
test cannot fail before the fix: its whole purpose is to pin the pre-fix
numbers. Second, #105's substance is a compile-time portability change plus a
guard on a branch that is unreachable through
expected_mi()'s own arithmeticonce #104's length check is in place; its test exercises the replacement
initialiser across both the table and
lgamma()branches ofl2factorial(),and fails pre-fix for #91's reason rather than #105's.
Local:
test-expected-mi.R28/28 pass,test-Concordance.R75/75 pass (2 CRANskips), against a tarball build installed to a per-agent library.
spelling::spell_check_package(vignettes = TRUE)clean. No test uses threads orcosts more than a second.
Reviewed, not fixed here
mi_key()narrows block sizes touint16_t, so.ExpectedMICachekeys aliasabove 65 535 items. Pre-existing, out of reach of the tree sizes this patch
targets, and touching it would change a signature — left alone deliberately.