Scalable contract: scale returns log Jacobian; sum-of-margins for Tree#70
Open
Scalable contract: scale returns log Jacobian; sum-of-margins for Tree#70
Conversation
Refactors the Scalable interface to a three-method contract whose
invariants are mutually consistent on a single dilation axis:
scale(s) returns log Jacobian determinant of the move
getScalableValue() returns position on the dilation axis
setScalableValue(V) default: scale(V / getScalableValue())
The contract requires:
(1) scale(s) makes getScalableValue() return s × original
(2) setScalableValue(V) makes getScalableValue() return V
(3) setScalableValue(get × s) produces the same state as scale(s)
A new ScalableContractTest harness exercises these three invariants on
every concrete implementation. Per-class tests cover RealScalarParam,
RealVectorParam, and Tree on ultrametric, heterochronous, and
leaf-intruding fixtures.
Tree.scale switches from affine internal-height scaling to interval
(margin) scaling so the move always succeeds for any positive s on
heterochronous trees, and the dilation summary getScalableValue =
sum of margins is exactly s-equivariant. The previous affine semantics
remain available as Tree.scaleToRootHeight(targetHeight), used by
StarBeastStartState init code.
Operator-side updates are mechanical: scale return type int -> double
log Jacobian, summed directly. ScaleOperator and UpDownOperator (legacy
+ spec + Bactrian variants) preserve their existing kernel-symmetry
corrections. AMVN routes through setScalableValue / getScalableValue,
inheriting the contract; this also fixes a latent bug in spec AMVN
where Tree.getValue had no return statement.
Out of scope for this PR (operator-design layer, deferred to follow-up):
* spec UpDown actualScaler logic for tree+up/down combinations
* IntervalScaleOperator collapse into UpDown
* AMVN's covariance behaviour for non-ultrametric trees beyond the
target-landing fix
Tested:
* 408 existing beast-base unit tests pass
* 9 new Scalable contract tests pass
* 10 heterochronous MCMC integration tests pass (TipTimeTest legacy
+ spec under -Pslow-tests, ~4 minutes)
* Mascot, beast-classic, LPhyBeast core compile cleanly against new
beast-base
* BEASTLabs requires corresponding 2-file update; see parallel
BEASTLabs scalable-contract branch (all 83 tests pass)
API impact: binding interface change. Downstream packages that
implement Scalable directly or store the int return must update; the
beast.base implementations and BEASTLabs branch demonstrate the
migration pattern.
Closes/addresses: #20
1 task
Same change pattern as TreeTest and the legacy ScaleOperatorTest in the parent commit. BactrianScaleOperatorTest is @tag("slow") so it was not exercised in the default test run; surfaced when running mvn -Pslow-tests test.
UpDownOperatorTest.testLogNormalDistribution and
RealRandomWalkOperatorTest.testNormalDistribution were failing
intermittently because their tolerance (5e-3) was at ~2.7 SE of the
sample mean — a ~5%/~10% expected failure rate per run by design.
Computing 3 SE properly using ESS rather than nominal sample count:
UpDownOperatorTest (LogNormal M=1, S=1, no tree, ~498k samples,
ESS near nominal): SE = sqrt(1.72 / 498000) ~= 1.86e-3, 3 SE ~= 5.6e-3.
Tolerance set to 6e-3.
RealRandomWalkOperatorTest (Normal mean=1, var=1, documented Mirror
ESS = 196k from existing comment): SE = sqrt(1 / 196000) ~= 2.26e-3,
3 SE ~= 6.8e-3. Tolerance set to 7e-3.
Re-enabled Randomizer.setSeed(127) in RealRandomWalkOperatorTest
(it had been commented out, leaving the test non-deterministic
against whatever Randomizer state preceded it).
Expected failure rate at the new bounds: 0.27% per test run.
These are pre-existing flake fixes surfaced when running mvn -Pslow-tests
test against the Scalable contract change (#70). The
identical failing values reproduce on master; not caused by the contract
change.
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.
Summary
Refactors
beast.base.inference.Scalableto a three-method contract whose invariants are mutually consistent on a single dilation axis:The contract requires (and a new test harness enforces):
scale(s)makesgetScalableValue()returns × original(scale-equivariance).setScalableValue(V)makesgetScalableValue()returnV(set is a fixed point of get).setScalableValue(get × s)produces the same state asscale(s)(set composes with scale).Tree.scaleswitches from affine internal-height scaling to interval (margin) scaling, so the move always succeeds for any positiveson heterochronous trees and the dilation summarygetScalableValue= sum of margins is exactlys-equivariant. The previous affine semantics remain available asTree.scaleToRootHeight(targetHeight), used byStarBeastStartStateinit code, outside the contract.Operator-side updates are mechanical:
scalereturn typeint → doublelog Jacobian, summed directly.ScaleOperatorandUpDownOperator(legacy + spec + Bactrian) preserve their existing kernel-symmetry corrections. AMVN routes throughsetScalableValue/getScalableValue, inheriting the contract.Closes #20 for the contract layer.
Why
Joelle Barido-Sottani is blocked porting custom-tree packages to beast3 because the old
int dofreturn onScalable.scalecouldn't express non-affine HRs and the specUpDownOperatorwas bypassingTree.scaleentirely (her March 26 comment on #20). The contract refactor is the fix: eachScalablenow declares a single dilation axis, the three methods are mutually consistent or the contract test fails, andgetScalableValuecarries whatever summary the implementer'sscaleactually preserves.The
Tree.scalebody change addresses a separate latent bug: the old affineNode.scalethrowsIllegalArgumentExceptionon heterochronous trees for anysbelow a tree-shape-dependent threshold (e.g., on a tree with leaves at h=0 and h=2 and a parent at h=4, anys < 0.5throws). Interval scaling preserves tip dates by construction and never throws fors > 0. dof equals numIntervals for any binary tree (with or without sampled ancestors), so operator HRs are unchanged.Out of scope (deferred to follow-ups)
These are operator-design questions, separable from the interface contract:
UpDownOperator'sactualScaler = lengthAfter / lengthBeforelogic for tree + up/down combinations (lines 158-176) — preserved as-is.IntervalScaleOperatorshould collapse intoUpDownOperatoronceTree.scaleis interval-scaling.Vunder interval scaling, but the empirical covariance choice is a separate discussion.)sor an "effective" tree-dilation factor for non-tree up/down parameters.API impact
This is a binding interface change. Downstream packages that implement
Scalabledirectly or store the int return must update:Scalableusage; compile cleanly against this branch.PrevalenceList,TreeScaleOperator) need a corresponding update. Parallel PR: Adapt to beast-base Scalable contract change BEAST2-Dev/BEASTLabs#29 on the matchingscalable-contractbranch (all 83 BEASTLabs tests pass against this beast3 branch).The
Scalable.scaleAlldefault helper is removed (no callers in any tracked repo).Test plan
Scalablecontract tests pass (ScalableContractTestharness + per-class tests forRealScalarParam,RealVectorParam, andTreeon ultrametric, heterochronous, and leaf-intruding fixtures)-Pslow-tests(TipTimeTestlegacy + spec, ~4 minutes; chains converge to BEAST1 reference values within tolerance)beast-baseSide-effect bug fixes
Spec
AdaptableVarianceMultivariateNormalOperator.getValuefor trees wast.getRoot().getHeight();with noreturnstatement, so it always fell through tothrow new RuntimeException("programmer error: should not get here"). The migration tot.getScalableValue()fixes the missing return.Notes for review
Scalable-instance, not per-operator. Operators remain free to choose what to do withscale(s)'s return — e.g.,UpDownOperatorkeeps its−2 log(s)kernel correction; specUpDownOperatordoes not. The contract just guarantees thatscale,get, andseton a single Scalable are mutually consistent.Node.intervalScale(new) sits alongsideNode.scale(unchanged).Tree.scalecalls the former;Tree.scaleToRootHeightcalls the latter. Two recursions, two purposes, no overlap.IntervalScaleOperator.resampleNodeHeight(the existing reference implementation we lifted).