Tree.scale / Node.intervalScale unsafe on fake-rooted SA trees

[alexeid]

Summary

When a tree's root is a "fake" node (the parent of a sampled ancestor sitting on the root branch), Tree.scale(double) — which delegates to Node.intervalScale(double) — can produce a topologically invalid tree, and the spec ScaleTreeOperator likewise has no protection on its rootOnly branch.

Where it goes wrong

Node.intervalScale (beast.base.evolution.tree.Node):

public int intervalScale(final double scale) {
    if (isLeaf()) return 0;
    // sampled-ancestor fake nodes: skip, recurse into the non-direct-ancestor child
    if (isFake()) {
        if (getLeft().isDirectAncestor()) return getRight().intervalScale(scale);
        else                              return getLeft().intervalScale(scale);
    }
    // ...non-fake branch: scales the margin, recomputes height after children scale
}

For a fake root:

• root.height stays pinned to the SA child's height (correct, that's the SA invariant).
• But the non-DA subtree's margins are scaled. For scale > 1, the non-DA child's height can rise above root.height, leaving a child taller than its parent. Topology is broken; downstream code that assumes the invariant will misbehave.

The fake-node branch has no post-recursion check, and the non-fake branch (which does recompute parent.height from children) never runs when the root itself is fake.

What the legacy code did

Legacy Node.scale (the historical affine path) had a defensive check at the end of the recursion:

if (height < getLeft().height || height < getRight().height) {
    throw new IllegalArgumentException("Scale gives negative branch length");
}

For a fake root with scale > 1, that fired and sa.evolution.operators.SAScaleOperator's try { ... } catch (Exception e) { return Double.NEGATIVE_INFINITY; } turned it into a clean rejection. The new intervalScale dropped that check, so the protection is gone.

ScaleTreeOperator's rootOnly path is also unsafe

beast.base.spec.evolution.operator.ScaleTreeOperator.proposal() writes root.setHeight(newHeight) directly in the rootOnly branch, bypassing intervalScale entirely. Its only safeguard is:

if (newHeight < Math.max(root.getLeft().getHeight(), root.getRight().getHeight()))
    return Double.NEGATIVE_INFINITY;

For a fake root, root.height == max(left, right) already, so that check catches scale < 1 but not scale > 1. Scaling the root up breaks the SA invariant (root height no longer equals the SA leaf's height).

The legacy SAScaleOperator rejected rootOnly moves outright when root.isFake(). That guard is currently the only thing protecting the spec base from the same defect, and the migration to spec drops it.

Proposed fix

Reasserting the legacy invariant in Node.intervalScale covers both code paths and makes downstream operators correct by default. After recursing into the non-DA child of a fake node, if the resulting child height exceeds this node's (pinned) height, the move is invalid; signal failure (throw, or return a sentinel) so Tree.scale can translate it to a -inf log-Hastings. The spec ScaleTreeOperator's rootOnly branch needs the same guard for fake roots: scaling root height up breaks the SA invariant, so reject when root.isFake() && scale > 1.

Reversibility holds for the truncated kernel: starting from a valid x, accepted moves only land on a valid x', and applying scale 1/s to x' lands back on x. With a symmetric scale kernel (q(s) = q(1/s) in log-space) the standard dof * log(s) Hastings ratio is correct on the valid region.

Why this matters now

sa.evolution.operators.SAScaleOperator in CompEvol/sampled-ancestors is one of the last legacy-base classes (extends ScaleOperator) blocking that package's BEAST 3 migration. With this fix, SAScaleOperator collapses to an empty subclass (or disappears entirely if the XML/fxtemplate references are updated to use spec ScaleTreeOperator directly).