Skip to content

Make spec RandomTree recognise spec MRCAPrior constraints - #121

Merged
walterxie merged 2 commits into
masterfrom
fix/spec-randomtree-mrcaprior
Jul 23, 2026
Merged

Make spec RandomTree recognise spec MRCAPrior constraints#121
walterxie merged 2 commits into
masterfrom
fix/spec-randomtree-mrcaprior

Conversation

@alexeid

@alexeid alexeid commented Jul 23, 2026

Copy link
Copy Markdown
Member

Problem

beast.base.spec.evolution.tree.coalescent.RandomTree still imported the legacy beast.base.evolution.tree.MRCAPrior. The spec MRCAPrior extends Distribution directly rather than subclassing the legacy class, so the instanceof MRCAPrior scan over the tree's outputs never matched and monophyletic constraints were silently dropped when initialising the starting tree.

Any calibrated analysis then starts from a random tree that violates its own monophyly constraints:

Start likelihood: -Infinity after 10 initialisation attempts
    P(HumanGorilla.prior) = -Infinity
    P(MonkeyApe.prior) = NaN  **
    ...
java.lang.RuntimeException: Could not find a proper state to initialise. Perhaps try another seed.

Reproduced on seeds 1, 7, 42 and 12345 — it is not seed-dependent.

There was no XML-level workaround: calibrationsInput was typed to the legacy class too, so an explicit <constraint idref="..."/> would fail type-checking, and legacy MRCAPrior.distInput requires a ParametricDistribution while spec priors are ScalarDistribution — incompatible hierarchies.

Impact

This is not limited to one package. beast-fx/src/main/resources/beast.fx/fxtemplates/Standard.xml pairs spec RandomTree with spec MRCAPrior, so every calibrated analysis produced by BEAUti under BEAST 3 is affected.

Fix

Switch the import, calibrationsInput and the distributions list to the spec MRCAPrior / ScalarDistribution types. ScalarDistribution.inverseCumulativeProbability returns a boxed value, so it is unboxed explicitly when computing the calibration bounds.

The legacy beast.base.evolution.tree.coalescent.RandomTree is untouched, so legacy XMLs keep working through the legacy class. No other caller passes MRCAPriors into the spec class — RandomTreeTest constructs one without constraints and RandomGeneTree does not touch the constraint machinery.

Verification

  • Full beast3 suite: BUILD SUCCESS, 527 tests (47 pkgmgmt + 458 base + 22 fx), 0 failures.
  • Found via bModelTest's examples/beast2-paper.xml (7 monophyletic clade calibrations), which failed to initialise on every seed before this change and now starts at a finite likelihood and samples normally:
    Start likelihood: -41052.324347071335
                  0    -41052.3243              N    -39681.3021     -1371.0222
              10000    -26682.0022         2.0       -26499.3254      -182.6768
    
  • The other two bModelTest examples were re-run as regression checks and are unaffected.

Second commit: LoggerTest isolation

CI on this branch went red on LoggerTest.testFileLog, which is unrelated to the change above — it fails or passes purely on where surefire schedules it relative to the integration tests. Three JVM-wide globals are set by those tests and never restored: file.name.prefix (XMLPathUtil.setUpOutputDir), Logger.FILE_MODE, and Logger.sampleOffset (ResumeTest).

Run ordering is not stable between CI runs, which is why master has been green:

Run prefix-setting tests LoggerTest Result
master 29714663831 69, 70, 71 57 (before) green
this branch 30044812322 26, 28, 30 58 (after) red

When LoggerTest runs second, openLogFile() prepends the stale prefix and writes to <prefix>beast.log while the test resolves beast.log against the working directory, so the existence check fails; and the stale sampleOffset is added to every logged sample, so the round-tripped sample number is off by the leaked offset.

Fixed by resetting all three in setUp(). Verified by forcing the ordering — with -Dsurefire.runOrder=reversealphabetical over ExampleXmlParsingTest, ResumeTest and LoggerTest, testFileLog fails on a clean tree before the change and passes after it, as it does under alphabetical and filesystem ordering. Full beast-base suite: 458 tests, 0 failures.

beast.base.spec.evolution.tree.coalescent.RandomTree still imported the
legacy beast.base.evolution.tree.MRCAPrior. Since the spec MRCAPrior
extends Distribution directly rather than subclassing the legacy class,
the `instanceof MRCAPrior` scan over the tree's outputs never matched,
so monophyletic constraints were silently dropped when initialising the
starting tree.

Any calibrated analysis then starts from a random tree that violates its
own monophyly constraints, giving -Infinity/NaN clade priors and
"Could not find a proper state to initialise". This affects every
calibrated analysis produced by BEAUti, whose Standard.xml template
pairs spec RandomTree with spec MRCAPrior.

Switch the import, calibrationsInput and the distributions list to the
spec MRCAPrior / ScalarDistribution types. ScalarDistribution returns a
boxed value from inverseCumulativeProbability, so unbox explicitly when
computing the calibration bounds.
@alexeid
alexeid force-pushed the fix/spec-randomtree-mrcaprior branch from c801678 to 28c76bf Compare July 23, 2026 21:05
@alexeid
alexeid requested a review from walterxie July 23, 2026 21:06
LoggerTest.testFileLog failed or passed depending purely on where
surefire happened to schedule it. Three JVM-wide globals are set by the
integration tests and never restored:

  - file.name.prefix   (XMLPathUtil.setUpOutputDir)
  - Logger.FILE_MODE   (ExampleXmlParsingTest, ResumeTest, ...)
  - Logger.sampleOffset (ResumeTest, via the resume path)

If LoggerTest runs after them, openLogFile() prepends the stale prefix
and writes to <prefix>beast.log while the test resolves "beast.log"
against the working directory, so the existence check fails; and the
stale sampleOffset is added to every logged sample, so the round-tripped
sample number is off by the leaked offset.

Reset all three in setUp(). sampleOffset is protected static and so is
not reachable from this package directly, but a subclass inherits it
regardless of package.

Verified by forcing the ordering: with -Dsurefire.runOrder=reversealphabetical
over ExampleXmlParsingTest, ResumeTest and LoggerTest, testFileLog failed
on a clean tree before this change and passes after it, as it does under
alphabetical and filesystem ordering.
@alexeid
alexeid requested a review from rbouckaert July 23, 2026 22:09
@walterxie
walterxie merged commit bc0a2bf into master Jul 23, 2026
1 check passed
alexeid added a commit to BEAST2-Dev/bModelTest that referenced this pull request Jul 23, 2026
beta7 is the first release containing the spec RandomTree fix
(CompEvol/beast3#121), so the example XMLs no longer need a locally
built SNAPSHOT and Maven Central will accept the release.

Verified against the artifacts as published: 31/31 tests pass and all
three example XMLs start at a finite likelihood and sample.

version.xml already declares atleast='2.8.0', which matches the BEAST
package version beast3 ships, so it needs no change -- the Maven
artifact version and the BEAST package version are separate axes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants