Fix -wslr/-wslm .sitelh invariant under per-category (safe_numeric) scaling#534
Open
famulare wants to merge 1 commit into
Open
Fix -wslr/-wslm .sitelh invariant under per-category (safe_numeric) scaling#534famulare wants to merge 1 commit into
-wslr/-wslm .sitelh invariant under per-category (safe_numeric) scaling#534famulare wants to merge 1 commit into
Conversation
writeSiteLh (via computePatternLikelihood) documents the invariant
"sum of exp(LnLW_k) is equal to exp(LnL)". It is violated on
numerically-extreme sites whenever the likelihood kernel runs with
per-category scaling (safe_numeric == true) but the per-category
writer takes its per-pattern scaling branch.
The kernel sets safe_numeric in setLikelihoodKernel() as:
(lk_safe_scaling || leafNum >= numseq_safe_scaling)
|| (num_states != 4 && num_states != 20)
i.e. codon / morphology / other non-DNA-non-protein alphabets always
use per-category scaling and lay scale_num out per category
(scale_num[ptn*ncat_mix + c]).
computePatternLikelihood() chose its per-category-vs-per-pattern branch
using only (lk_safe_scaling || leafNum >= numseq_safe_scaling), omitting
the num_states clause. So for e.g. a 61-state codon model the writer
read scale_num as a per-pattern array (scale_num[ptn]), mis-indexing it
and applying a single wrong scale step uniformly to all categories. On
sites deep enough for the categories to underflow at different scale
counts, every LnLW_k came out exactly LOG_SCALING_THRESHOLD
(-177.4457) too low, so sum exp(LnLW_k) < exp(LnL).
Fix: select the writer branch with the tree's own safe_numeric flag,
which is the authoritative record of the kernel scaling path used.
DNA/protein (4/20-state) output is byte-identical.
Co-Authored-By: Claude Opus 4.8 (1M context) <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 the
.sitelhinvariant documented in the file's own header:Fixes #533. See that issue for the full root-cause write-up and reproduction.
The bug
PhyloTree::computePatternLikelihood()writes the per-rate/mixture-categorysite log-likelihoods (
-wslr/-wslm/-wslmr). It chose between per-categoryand per-pattern scaling with:
if (params->lk_safe_scaling || leafNum >= params->numseq_safe_scaling)But the likelihood kernel decides its scaling mode via
PhyloTree::safe_numeric(set in
setLikelihoodKernel()), which is broader:safe_numeric = (params && (params->lk_safe_scaling || leafNum >= params->numseq_safe_scaling)) || (aln && aln->num_states != 4 && aln->num_states != 20);The two conditions differ only on the
num_statesclause, so they disagreeexactly when the writer condition is false but
safe_numericis true: anon-4-state-DNA / non-20-state-protein alphabet (codon, morphology, custom),
run without
-safe, on a tree belownumseq_safe_scaling(default2000) tips. In that case the kernel scales per category and stores
scale_numper category (scale_num[ptn*ncat_mix + c]), while the writer tookits per-pattern branch and read
scale_numas a flat per-pattern array(
scale_num[ptn]). This mis-indexes the scale counts and applies a single,wrong scale step uniformly to all categories. On numerically-extreme sites
(categories underflowing at different scale counts) every
LnLW_kcomes outexactly
LOG_SCALING_THRESHOLD(≈ 177.4457) too low, breaking the invariant.(
-safeor a ≥2000-tip tree make the writer condition true as well, so theymask the bug rather than cause it.)
.siteprobis unaffected because itsposteriors are normalized.
The fix
Use the tree's own
safe_numericflag to select the writer branch, so thewriter always matches the kernel's actual scaling layout.
double *lh_cat = _pattern_lh_cat; double *out_lh_cat = ptn_lh_cat; UBYTE *nei2_scale = nei2->scale_num; - if (params->lk_safe_scaling || leafNum >= params->numseq_safe_scaling) { + // NOTE: must match the kernel's per-category vs per-pattern scaling choice + // (PhyloTree::safe_numeric, set in setLikelihoodKernel), otherwise scale_num + // is mis-indexed and the per-category log-likelihoods are off by a scale step. + if (safe_numeric) { // per-category scaling(same change in the external-branch and internal-branch blocks; two
ifconditions).
Verification
Reproduction (codon, deep branches, 5 well-separated FreeRate categories,
seed 7 — fully self-contained via
--alisim, see the companion issue for thecommands and the verification script):
Example site 85, before → after:
Regression / no-change checks:
GTR+F+R4, 4-state, per-pattern path):.sitelhisbyte-identical before and after the patch.
example/example.phy(
GTR+R4 -wslr): invariant holds (max|dev| ≈ 9e-5).MK+R) and profile-mixture (-wslmr) codon runs: invariantholds (0 violations).
-10226.6026before and after),and
sum_i LnL_istill equals the tree log-likelihood — confirming thechange touches only the per-category reporting, not the likelihood engine.
Scope
Two-line logic change in
tree/phylotree.cpp(plus comments). No behaviorchange for DNA/protein; no change to the likelihood engine, tree search, or
any other output.
AI-assistance disclosure: this fix was diagnosed and drafted with the help
of Claude Opus 4.8. All changes were reviewed and verified by Mike Famulare
before submission.