From a45bd0b0ecbfc358a7b0c9a0f67702520a317e18 Mon Sep 17 00:00:00 2001 From: walterxie Date: Mon, 7 Sep 2026 15:32:44 +1200 Subject: [PATCH] fix hyperprior in beauti #157 --- .../beastfx/app/inputeditor/BeautiConfig.java | 36 ++-- .../inputeditor/spec/ScalarInputEditor.java | 59 ++++-- .../beastfx/app/beauti/HyperPriorTest.java | 194 ++++++++++++++++++ 3 files changed, 254 insertions(+), 35 deletions(-) create mode 100644 beast-fx/src/test/java/test/beastfx/app/beauti/HyperPriorTest.java diff --git a/beast-fx/src/main/java/beastfx/app/inputeditor/BeautiConfig.java b/beast-fx/src/main/java/beastfx/app/inputeditor/BeautiConfig.java index d22cb2d0..06c05be3 100644 --- a/beast-fx/src/main/java/beastfx/app/inputeditor/BeautiConfig.java +++ b/beast-fx/src/main/java/beastfx/app/inputeditor/BeautiConfig.java @@ -1,25 +1,20 @@ package beastfx.app.inputeditor; - -import java.io.File; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Set; - -import beastfx.app.util.Alert; -import beastfx.app.util.FXUtils; -import javafx.scene.Parent; import beast.base.core.BEASTInterface; import beast.base.core.BEASTObject; import beast.base.core.Description; import beast.base.core.Input; import beast.base.core.Input.Validate; import beast.base.evolution.alignment.Alignment; -import beast.base.spec.evolution.tree.MRCAPrior; import beast.base.parser.XMLParser; +import beast.base.spec.evolution.tree.MRCAPrior; +import beastfx.app.util.Alert; +import beastfx.app.util.FXUtils; +import javafx.scene.Parent; + +import java.io.File; +import java.util.*; @Description("Beauti configuration object, used to find Beauti configuration " + "information from Beauti template files.") @@ -131,17 +126,18 @@ public void initAndValidate() { } } - final static String HYPER_PRIOR_XML = - " \n" + - " \n" + - " \n" + + final static String HYPER_PRIOR_XML = + " \n" + + " \n" + + " \n" + " \n" + - " \n" + + " \n" + + " \n" + + " \n" + " \n" + "\n" + - " \n" + + " \n" + " ]]>\n" + " \n" + "\n" + diff --git a/beast-fx/src/main/java/beastfx/app/inputeditor/spec/ScalarInputEditor.java b/beast-fx/src/main/java/beastfx/app/inputeditor/spec/ScalarInputEditor.java index df09487d..da630e0e 100644 --- a/beast-fx/src/main/java/beastfx/app/inputeditor/spec/ScalarInputEditor.java +++ b/beast-fx/src/main/java/beastfx/app/inputeditor/spec/ScalarInputEditor.java @@ -5,7 +5,6 @@ import beast.base.core.BEASTInterface; import beast.base.core.Input; import beast.base.core.Log; -import beast.base.inference.Distribution; import beast.base.inference.Operator; import beast.base.inference.StateNode; import beast.base.parser.PartitionContext; @@ -273,25 +272,50 @@ public void toggleEstimate() { } String id = parameter2.getID(); - - if (id.startsWith("RealParameter")) { - ScalarDistribution parent = null; + // Has a hyperprior already been created for this parameter? True if one of + // parameter2's outputs is a ScalarDistribution whose own param is parameter2 + // (a hyperprior is always wired that way). Not true for the distribution + // parameter2 is itself a hyperparameter *of* (e.g. Normal, if parameter2 is + // "mean"): that Normal's param points at what IT is a prior for (e.g. + // birthRate), not at parameter2. + boolean hasHyperPrior = false; + for (Object beastObject2 : parameter2.getOutputs()) { + if (beastObject2 instanceof ScalarDistribution sd && sd.paramInput.get() == parameter2) { + hasHyperPrior = true; + break; + } + } + + // First time for this parameter: find the distribution it's a hyperparameter + // of, then rename it so a hyperprior can be created for it and found again + // on every later call (see the rename below). + if (!hasHyperPrior) { + ScalarDistribution parent = null; for (Object beastObject2 : parameter2.getOutputs()) { - if (beastObject2 instanceof ScalarDistribution) { - parent = (ScalarDistribution) beastObject2; + // the distribution parameter2 is itself a hyperparameter of (e.g. + // Normal, if parameter2 is its "mean"). It's the only kind of + // ScalarDistribution parameter2 can have among its outputs here, since + // hasHyperPrior above already ruled out one whose "param" is parameter2. + if (beastObject2 instanceof ScalarDistribution sd) { + parent = sd; break; } } - Distribution grandparent = null; - for (Object beastObject2 : parent.getOutputs()) { - if (beastObject2 instanceof Distribution) { - grandparent = (Distribution) beastObject2; - break; - } + if (parent == null) { + // shouldn't happen given isParametricDistributionParameter, but defend + // against an inconsistent model rather than dereferencing a null parent + Log.err.println("Could not find the distribution owning " + parameter2.getID() + "; not adding a hyperprior"); + m_isEstimatedBox.setSelected(false); + parameter2.isEstimatedInput.setValue(false, parameter2); + return; } - id = "parameter.hyper" + parent.getClass().getSimpleName() + "-" + - m_input.getName() + "-" + grandparent.getID(); + + // rename from the auto-generated id (e.g. "RealScalarParam.N") to + // "parameter.", which the $(n) substitution below, and BeautiDoc's + // general "parameter.*" sync scan (see BeautiDoc.scrubAll), rely on to find it + id = "parameter.hyper" + parent.getClass().getSimpleName() + "-" + + m_input.getName() + "-" + parent.getID(); doc.pluginmap.remove(parameter2.getID()); parameter2.setID(id); doc.addPlugin(parameter2); @@ -300,12 +324,17 @@ public void toggleEstimate() { PartitionContext context = new PartitionContext(id.substring("parameter.".length())); Log.warning.println(context + " " + id); + // ensures the hyperprior's own objects (HyperPrior.$(n), hyperScaler.$(n)) + // exist -- a safe no-op if they already do. Connecting/disconnecting them + // (state, operator, tracelog, the priors list) based on the current estimate + // value is done separately, by BeautiDoc's general "parameter.*" sync scan + // (see BeautiDoc.scrubAll), which hardSync()/refreshPanel() below triggers. doc.beautiConfig.hyperPriorTemplate.createSubNet(context, true); } hardSync(); refreshPanel(); } catch (Exception ex) { - Log.err.println("ParameterInputEditor " + ex.getMessage()); + Log.err.println("ScalarInputEditor " + ex.getMessage()); } } diff --git a/beast-fx/src/test/java/test/beastfx/app/beauti/HyperPriorTest.java b/beast-fx/src/test/java/test/beastfx/app/beauti/HyperPriorTest.java new file mode 100644 index 00000000..8f2bcf39 --- /dev/null +++ b/beast-fx/src/test/java/test/beastfx/app/beauti/HyperPriorTest.java @@ -0,0 +1,194 @@ +package test.beastfx.app.beauti; + + +import beast.base.inference.CompoundDistribution; +import beast.base.inference.Distribution; +import beast.base.spec.inference.distribution.LogNormal; +import beast.base.spec.inference.distribution.LogUniform; +import beastfx.app.beauti.BeautiTabPane; +import javafx.scene.control.ComboBox; +import javafx.stage.Stage; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.testfx.api.FxRobot; +import org.testfx.framework.junit5.ApplicationExtension; +import org.testfx.framework.junit5.Start; +import org.testfx.service.query.NodeQuery; +import org.testfx.util.WaitForAsyncUtils; + +import java.io.File; +import java.util.List; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; + + +/** + * Checking "estimate" on a LogNormal prior's own parameter (M or S) in the Priors panel is meant + * to pop up a "Hyper prior" confirmation dialog, and, once confirmed, add a hyper prior (a + * LogUniform distribution) for that parameter as a new row in the Priors panel. This used to + * silently do nothing in the spec framework, because the parameter's auto-generated id + * ("RealScalarParam.N") never matched the legacy "RealParameter" prefix check that triggered + * renaming + hyper prior creation, and the legacy hyper prior object was also incompatible with + * spec-framework parameters -- so no row was ever added. + *

+ * The same "estimate" checkbox in the Site Model and Clock Model panels must never pop up the + * hyper prior dialog at all, since those parameters (e.g. mutationRate, clock.rate) are not a + * distribution's own hyperparameter -- see spec.ScalarInputEditor#isHyperparameter. + */ +@ExtendWith(ApplicationExtension.class) +public class HyperPriorTest extends BeautiBase { + + @Start + public void start(Stage stage) { + System.setProperty("beast.is.junit.testing", "true"); + try { + BeautiTabPane tabPane = BeautiTabPane.main2(new String[] {}, stage); + this.doc = tabPane.doc; + stage.show(); + } catch (Exception e) { + e.printStackTrace(); + } + } + + /** + * Both M and S are checked in a single test method (rather than one @Test each) because + * every "estimate" checkbox in this window shares its plain fx:id across BEAUti windows + * (e.g. "M.isEstimated" is not partition-qualified), and this test harness does not close a + * previous test method's window before the next one opens (see FixedMeanRateTest's comment + * on cascading windows) -- a second, separate @Test method here would risk resolving its + * checkbox lookup against the first method's now-stale window instead of its own. + */ + @Test + public void addHyperPriorForLogNormalMAndS(FxRobot robot) throws Exception { + switchBirthRatePriorToLogNormal(robot); + + // M is Real-domain, S is PositiveReal-domain: both get a LogUniform hyper prior + int priorCountBefore = priors().size(); + clickOnNodesWithID(robot, "M.isEstimated"); + WaitForAsyncUtils.waitForFxEvents(); + robot.clickOn("Yes"); + WaitForAsyncUtils.waitForFxEvents(); + + List priors = priors(); + assertThat(priors.size()).as("a hyper prior for M should have been added to the Priors panel") + .isEqualTo(priorCountBefore + 1); + LogUniform hyperPriorForM = (LogUniform) lastPrior(priors); + assertThat(hyperPriorForM.paramInput.get()).as("the hyper prior must apply to M itself") + .isSameAs(findLogNormal(priors).MParameterInput.get()); + + priorCountBefore = priors().size(); + clickOnNodesWithID(robot, "S.isEstimated"); + WaitForAsyncUtils.waitForFxEvents(); + robot.clickOn("Yes"); + WaitForAsyncUtils.waitForFxEvents(); + + priors = priors(); + assertThat(priors.size()).as("a hyper prior for S should have been added to the Priors panel") + .isEqualTo(priorCountBefore + 1); + LogUniform hyperPriorForS = (LogUniform) lastPrior(priors); + assertThat(hyperPriorForS.paramInput.get()).as("the hyper prior must apply to S itself") + .isSameAs(findLogNormal(priors).SParameterInput.get()); + + makeSureXMLParses(); + } + + @Test + public void siteModelEstimateNeverOffersHyperPrior(FxRobot robot) throws Exception { + importAlignment(NEXUS_DIR, new File("anolis.nex")); + WaitForAsyncUtils.waitForFxEvents(); + + selectTab(robot, "Site Model"); + WaitForAsyncUtils.waitForFxEvents(); + int priorCountBefore = priors().size(); + + // if this popped up the "Hyper prior" dialog, nothing here would dismiss it, and + // the assertion below would see a stale (unsynced) prior list + clickOnNodesWithID(robot, "mutationRate.isEstimated"); + WaitForAsyncUtils.waitForFxEvents(); + + assertThat(priors().size()).as("estimating a Site Model parameter must never add a hyper prior") + .isEqualTo(priorCountBefore); + } + + @Test + public void clockModelEstimateNeverOffersHyperPrior(FxRobot robot) throws Exception { + importAlignment(NEXUS_DIR, new File("anolis.nex")); + WaitForAsyncUtils.waitForFxEvents(); + + selectTab(robot, "Clock Model"); + WaitForAsyncUtils.waitForFxEvents(); + int priorCountBefore = priors().size(); + + clickOnNodesWithID(robot, "clock.rate.isEstimated"); + WaitForAsyncUtils.waitForFxEvents(); + + assertThat(priors().size()).as("estimating a Clock Model parameter must never add a hyper prior") + .isEqualTo(priorCountBefore); + } + + /** switch the birth rate's prior from the default Gamma to LogNormal, then expand its row so + * its own M (Real-domain) and S (PositiveReal-domain) parameters -- collapsed by default, + * like every prior row -- render their "estimate" checkboxes. LogNormal itself is + * PositiveReal-domain, matching birthRate -- Normal (Real-domain) is not a domain-compatible + * option for birthRate, so this is not just an arbitrary choice. */ + private void switchBirthRatePriorToLogNormal(FxRobot robot) throws Exception { + importAlignment(NEXUS_DIR, new File("anolis.nex")); + WaitForAsyncUtils.waitForFxEvents(); + + selectTab(robot, "Priors"); + WaitForAsyncUtils.waitForFxEvents(); + + selectDistribution(robot, "birthRate.t:anolis.distr", "Log Normal"); + WaitForAsyncUtils.waitForFxEvents(); + + Distribution birthRatePrior = null; + for (Distribution d : priors()) { + if (d instanceof LogNormal) { + birthRatePrior = d; + } + } + assertThat(birthRatePrior).as("birth rate prior should now be a LogNormal").isNotNull(); + + clickOnNodesWithID(robot, birthRatePrior.getID() + ".editButton"); + WaitForAsyncUtils.waitForFxEvents(); + } + + /** select an item (matched by its combo-box display text, e.g. "Log Normal") from a + * distribution ComboBox, driving the ComboBox API directly rather than the dropdown UI + * (avoids the ComboBox's popup window sticking open, which would then swallow later clicks). */ + @SuppressWarnings("unchecked") + private void selectDistribution(FxRobot robot, String comboBoxId, String displayText) { + NodeQuery q = robot.lookup(target -> comboBoxId.equals(target.getId()) && target.isVisible()); + Set nodes = q.queryAll(); + assertThat(nodes).as("no visible ComboBox with id " + comboBoxId).isNotEmpty(); + ComboBox comboBox = (ComboBox) nodes.iterator().next(); + robot.interact(() -> { + for (Object item : comboBox.getItems()) { + if (item.toString().equals(displayText)) { + comboBox.getSelectionModel().select(item); + return; + } + } + throw new AssertionError("No item \"" + displayText + "\" in ComboBox #" + comboBoxId); + }); + } + + private List priors() { + CompoundDistribution prior = (CompoundDistribution) doc.pluginmap.get("prior"); + return prior.pDistributions.get(); + } + + private Distribution lastPrior(List priors) { + return priors.get(priors.size() - 1); + } + + private LogNormal findLogNormal(List priors) { + for (Distribution d : priors) { + if (d instanceof LogNormal ln) { + return ln; + } + } + throw new AssertionError("No LogNormal found among the priors"); + } +}