From 1da8a9af7b0473f0e26ee0bfd1a90a01b591c96a Mon Sep 17 00:00:00 2001 From: walterxie Date: Mon, 31 Aug 2026 15:47:41 +1200 Subject: [PATCH 1/6] use new (spec) input editors to replace legacy editors #162 --- .../base/inference/parameter/Parameter.java | 1 + .../beastfx/app/inputeditor/BeautiDoc.java | 1 + .../app/inputeditor/ParameterInputEditor.java | 8 +++-- .../app/inputeditor/SiteModelInputEditor.java | 4 +++ beast-fx/src/main/java/module-info.java | 2 +- .../beast.fx/fxtemplates/Standard.xml | 2 +- .../beast.fx/fxtemplates/SubstModels.xml | 2 +- .../beastfx/app/beauti/FixedMeanRateTest.java | 32 +++++++++++++------ beast-fx/version.xml | 3 +- 9 files changed, 39 insertions(+), 16 deletions(-) diff --git a/beast-base/src/main/java/beast/base/inference/parameter/Parameter.java b/beast-base/src/main/java/beast/base/inference/parameter/Parameter.java index e8ea57ca..0fc07659 100644 --- a/beast-base/src/main/java/beast/base/inference/parameter/Parameter.java +++ b/beast-base/src/main/java/beast/base/inference/parameter/Parameter.java @@ -141,6 +141,7 @@ default List getKeysList() { */ void swap(int i, int j); + @Deprecated @Description("A parameter represents a value in the state space that can be changed by operators.") abstract class Base extends StateNode implements Parameter { diff --git a/beast-fx/src/main/java/beastfx/app/inputeditor/BeautiDoc.java b/beast-fx/src/main/java/beastfx/app/inputeditor/BeautiDoc.java index b56771c8..fbecc9ca 100644 --- a/beast-fx/src/main/java/beastfx/app/inputeditor/BeautiDoc.java +++ b/beast-fx/src/main/java/beastfx/app/inputeditor/BeautiDoc.java @@ -33,6 +33,7 @@ import beast.pkgmgmt.PackageManager; import beast.pkgmgmt.launcher.BeastLauncher; import beastfx.app.beauti.BeautiTabPane; +import beastfx.app.inputeditor.spec.SiteModelInputEditor; import beastfx.app.util.Alert; import beastfx.app.util.FXUtils; import beastfx.app.util.PartitionContextUtil; diff --git a/beast-fx/src/main/java/beastfx/app/inputeditor/ParameterInputEditor.java b/beast-fx/src/main/java/beastfx/app/inputeditor/ParameterInputEditor.java index f1d8fbf4..a5c01383 100644 --- a/beast-fx/src/main/java/beastfx/app/inputeditor/ParameterInputEditor.java +++ b/beast-fx/src/main/java/beastfx/app/inputeditor/ParameterInputEditor.java @@ -24,9 +24,11 @@ import java.util.List; - - - +/** + * @deprecated replaced by {@link beastfx.app.inputeditor.spec.ScalarInputEditor} + * or {@link beastfx.app.inputeditor.spec.VectorInputEditor} + */ +@Deprecated public class ParameterInputEditor extends BEASTObjectInputEditor { boolean isParametricDistributionParameter = false; diff --git a/beast-fx/src/main/java/beastfx/app/inputeditor/SiteModelInputEditor.java b/beast-fx/src/main/java/beastfx/app/inputeditor/SiteModelInputEditor.java index 6c294543..dfeb5165 100644 --- a/beast-fx/src/main/java/beastfx/app/inputeditor/SiteModelInputEditor.java +++ b/beast-fx/src/main/java/beastfx/app/inputeditor/SiteModelInputEditor.java @@ -26,6 +26,10 @@ import java.util.ArrayList; import java.util.List; +/** + * @deprecated replaced by {@link beastfx.app.inputeditor.spec.SiteModelInputEditor} + */ +@Deprecated public class SiteModelInputEditor extends BEASTObjectInputEditor { IntegerInputEditor categoryCountEditor; diff --git a/beast-fx/src/main/java/module-info.java b/beast-fx/src/main/java/module-info.java index 30aee9a8..c769582f 100644 --- a/beast-fx/src/main/java/module-info.java +++ b/beast-fx/src/main/java/module-info.java @@ -74,7 +74,7 @@ beastfx.app.inputeditor.LongInputEditor, beastfx.app.inputeditor.OutFileInputEditor, beastfx.app.inputeditor.OutFileListInputEditor, - beastfx.app.inputeditor.ParameterInputEditor, + // ScalarInputEditor and VectorInputEditor replace ParameterInputEditor beastfx.app.inputeditor.ParametricDistributionInputEditor, // The legacy SiteModelInputEditor is intentionally NOT provided: the spec // SiteModelInputEditor (below) is the BEAUti editor for SiteModelInterface.Base in diff --git a/beast-fx/src/main/resources/beast.fx/fxtemplates/Standard.xml b/beast-fx/src/main/resources/beast.fx/fxtemplates/Standard.xml index 8b3a239a..b7d644a9 100644 --- a/beast-fx/src/main/resources/beast.fx/fxtemplates/Standard.xml +++ b/beast-fx/src/main/resources/beast.fx/fxtemplates/Standard.xml @@ -428,7 +428,7 @@ ClusterTree/clusterType/, - + diff --git a/beast-fx/src/main/resources/beast.fx/fxtemplates/SubstModels.xml b/beast-fx/src/main/resources/beast.fx/fxtemplates/SubstModels.xml index 39c08bc0..1f9b8895 100644 --- a/beast-fx/src/main/resources/beast.fx/fxtemplates/SubstModels.xml +++ b/beast-fx/src/main/resources/beast.fx/fxtemplates/SubstModels.xml @@ -82,7 +82,7 @@ MutationRatePrior/index/=Priors/MutationRatePrior/ ]]> - + diff --git a/beast-fx/src/test/java/test/beastfx/app/beauti/FixedMeanRateTest.java b/beast-fx/src/test/java/test/beastfx/app/beauti/FixedMeanRateTest.java index c2cc8cf1..a69b9582 100644 --- a/beast-fx/src/test/java/test/beastfx/app/beauti/FixedMeanRateTest.java +++ b/beast-fx/src/test/java/test/beastfx/app/beauti/FixedMeanRateTest.java @@ -1,20 +1,21 @@ package test.beastfx.app.beauti; -import static org.junit.jupiter.api.Assertions.assertEquals; - -import java.io.File; -import java.util.List; - +import beast.base.spec.inference.operator.DeltaExchangeOperator; +import beastfx.app.beauti.BeautiTabPane; +import javafx.scene.control.CheckBox; +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 beast.base.spec.inference.operator.DeltaExchangeOperator; -import beastfx.app.beauti.BeautiTabPane; -import javafx.stage.Stage; +import java.io.File; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; /** test how the FixedMeanRate flag interact with link/unlink **/ @ExtendWith(ApplicationExtension.class) @@ -53,6 +54,10 @@ public void testFixedMeanRate(FxRobot robot) throws Exception { clickOnButtonWithText(robot, "Link Site Models"); clickOnButtonWithText(robot, "Unlink Site Models"); + selectTab(robot, "Site Model"); + CheckBox fixMeanCheckBox = robot.lookup("#FixMeanMutationRate").queryAs(CheckBox.class); + assertTrue(fixMeanCheckBox.isSelected(), "Fix mean substitution rate checkbox should still be checked after link/unlink"); + //saveFile("/Users/remcobouckaert/tmp", "x.xml"); makeSureXMLParses(); @@ -80,7 +85,11 @@ public void testFixedMeanRateSharedSiteModel(FxRobot robot) throws Exception { selectPartitions(robot, 0, 1, 2); clickOnButtonWithText(robot, "Link Site Models"); clickOnButtonWithText(robot, "Unlink Site Models"); - + + selectTab(robot, "Site Model"); + CheckBox fixMeanCheckBox = robot.lookup("#FixMeanMutationRate").queryAs(CheckBox.class); + assertTrue(fixMeanCheckBox.isSelected(), "Fix mean substitution rate checkbox should be checked after link/unlink"); + DeltaExchangeOperator operator = (DeltaExchangeOperator) doc.pluginmap.get("FixMeanMutationRatesOperator"); int nrOfParameters = operator.parameterInput.get().size(); if (nrOfParameters != 3) { @@ -96,6 +105,11 @@ public void testFixedMeanRateSharedSiteModel(FxRobot robot) throws Exception { selectPartitions(robot, 0, 2); // beautiFrame.table().selectCells(TableCell.row(0).column(1), TableCell.row(2).column(1)); clickOnButtonWithText(robot, "Link Site Models"); + + selectTab(robot, "Site Model"); + fixMeanCheckBox = robot.lookup("#FixMeanMutationRate").queryAs(CheckBox.class); + assertTrue(fixMeanCheckBox.isSelected(), "Fix mean substitution rate checkbox should still be checked after second link"); + operator = (DeltaExchangeOperator) doc.pluginmap.get("FixMeanMutationRatesOperator"); nrOfParameters = operator.parameterInput.get().size(); diff --git a/beast-fx/version.xml b/beast-fx/version.xml index 691fad6b..4f51281a 100644 --- a/beast-fx/version.xml +++ b/beast-fx/version.xml @@ -55,7 +55,7 @@ - + - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -]]> - - - - - - - - - - - - - - - - - - - - - - - - - - - - - Scales proportion of invariant sites parameter of partition $(n) - Scales mutation rate of partition s:$(n) - Scales gamma shape parameter of partition s:$(n) - Scale clock rate of partition c:$(n) - Scales birth rate of Yule prior for partition t:$(n) - - Scale up substitution rate c:$(n) and scale down tree t:($n) - - - Estimates tip dates for tree t:$(n) - - - Scales root node for tree t:$(n) - Draws new internal node heights uniformally for tree t:$(n) - Performs subtree slide rearrangement of tree t:$(n) - Narrow exchange performs local rearrangement of tree t:$(n) - Wide exchange performs global rearrangement of tree t:$(n) - Performs Wilson-Balding global rearrangement of tree t:$(n) - Epoch operator that only scale internal nodes above least recent node for tree t:$(n) - Epoch operator that scales all internal nodes for tree t:$(n) - Flex operators for all internal nodes for tree t:$(n) - - - - - - - - - - - Prior on gamma shape for partition s:$(n) - Prior on proportion invariant for partition s:$(n) - Prior on Yule birth rate for partition s:$(n) - - - - - - - - - - - - - - - - - - - -]]> - - - - - - -]]> - - - - - -]]> - - - - - -]]> - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -]]> - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - From d379ddf93cbf1a748c295e0ac22c913d27795ec5 Mon Sep 17 00:00:00 2001 From: walterxie Date: Mon, 31 Aug 2026 16:23:06 +1200 Subject: [PATCH 3/6] use correct BranchModel.Base and recover estimate checkbox, but find id issue #157 #162 --- .../beastfx/app/inputeditor/spec/ScalarInputEditor.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) 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 055d0683..d3029986 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.evolution.branchratemodel.BranchRateModel; import beast.base.inference.Distribution; import beast.base.inference.Operator; import beast.base.inference.StateNode; @@ -189,8 +188,8 @@ protected void addComboBox(Pane box, Input input, BEASTInterface beastObject) boolean isClockRate = false; for (Object output : ((BEASTInterface)parameter).getOutputs()) { - if (output instanceof BranchRateModel.Base) { - isClockRate |= ((BranchRateModel.Base) output).meanRateInput.get() == parameter; + if (output instanceof beast.base.spec.evolution.branchratemodel.Base branchRateModel) { + isClockRate |= branchRateModel.meanRateInput.get() == parameter; } } m_isEstimatedBox.setDisable(!(!isClockRate || !getDoc().autoSetClockRate)); @@ -212,7 +211,7 @@ protected void addComboBox(Pane box, Input input, BEASTInterface beastObject) //m_bAddButtons = false; if (itemNr < 0) { for (Object beastObject2 : ((BEASTInterface) m_input.get()).getOutputs()) { - if (beastObject2 instanceof RealScalarParam) { + if (beastObject2 instanceof ScalarDistribution) { m_isEstimatedBox.setVisible(doc.allowLinking); m_isEstimatedBox.setVisible(true); isParametricDistributionParameter = true; From bac0d7f82708351e9e8b7dafa9161e872bd35417 Mon Sep 17 00:00:00 2001 From: walterxie Date: Mon, 31 Aug 2026 16:25:27 +1200 Subject: [PATCH 4/6] add TensorDistributionInputEditorTest #160 --- .../TensorDistributionInputEditorTest.java | 207 ++++++++++++++++++ 1 file changed, 207 insertions(+) create mode 100644 beast-fx/src/test/java/test/beastfx/app/inputeditor/TensorDistributionInputEditorTest.java diff --git a/beast-fx/src/test/java/test/beastfx/app/inputeditor/TensorDistributionInputEditorTest.java b/beast-fx/src/test/java/test/beastfx/app/inputeditor/TensorDistributionInputEditorTest.java new file mode 100644 index 00000000..8b3513f7 --- /dev/null +++ b/beast-fx/src/test/java/test/beastfx/app/inputeditor/TensorDistributionInputEditorTest.java @@ -0,0 +1,207 @@ +package test.beastfx.app.inputeditor; + + +import beast.base.core.Input; +import beast.base.inference.Distribution; +import beast.base.spec.domain.PositiveReal; +import beast.base.spec.domain.Real; +import beast.base.spec.inference.distribution.IID; +import beast.base.spec.inference.distribution.Normal; +import beast.base.spec.inference.parameter.RealScalarParam; +import beast.base.spec.inference.parameter.RealVectorParam; +import beast.base.spec.inference.parameter.SimplexParam; +import beastfx.app.inputeditor.*; +import javafx.application.Platform; +import javafx.scene.Node; +import javafx.scene.Parent; +import javafx.scene.control.ComboBox; +import javafx.stage.Stage; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.testfx.framework.junit5.ApplicationExtension; +import org.testfx.framework.junit5.Start; + +import java.lang.reflect.Field; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Collectors; + +import static org.junit.jupiter.api.Assertions.*; + + +/** + * Regression test for issue #160: a vector-valued parameter declared with an + * unconstrained {@code domain="Real"} -- e.g. Bayesian Skyline's population sizes, + * {@code } -- must not offer + * Dirichlet in the Priors panel's "distr" combo box, since Dirichlet only applies to + * a Simplex (elements constrained to [0,1] and summing to 1), not an arbitrary real + * vector. IID legitimately applies to a Real-domain vector, so it must still be + * offered there; conversely it must not be offered for a Simplex-valued parameter. + */ +@ExtendWith(ApplicationExtension.class) +public class TensorDistributionInputEditorTest { + + @Start + public void start(Stage stage) { + // nothing to show, only the JavaFX toolkit is required + } + + /** the editor caches templates in static fields, so do not leak them into other tests **/ + @BeforeEach + @AfterEach + public void clearTemplateCache() throws Exception { + for (String name : new String[] {"tensorTemplates", "templateInstances", "templateDomains"}) { + Field field = TensorDistributionInputEditor.class.getDeclaredField(name); + field.setAccessible(true); + field.set(null, null); + } + } + + + @Test + public void realDomainVectorDoesNotOfferDirichletButOffersIID() throws Exception { + // Bug (#160): a vector param declared with the generic, unconstrained "Real" + // domain -- BEAUti's default when no domain is given, e.g. Bayesian Skyline's + // popSizes: -- used to + // have Dirichlet offered as a compatible prior in the "distr" combo box. + // Dirichlet actually requires a Simplex (elements in [0,1] that sum to 1), + // which "Real" does not guarantee, so offering it there would let a user build + // an invalid model. + RealVectorParam popSizes = new RealVectorParam<>( + new double[] {380.0, 380.0, 380.0, 380.0, 380.0}, Real.INSTANCE); + + Set distrNames = distrNamesOfferedFor(popSizes, "bPopSizes.t:anolis"); + + // Dirichlet must not appear: it needs a Simplex-valued param, not a plain Real vector. + assertFalse(distrNames.contains("Dirichlet"), + "Dirichlet requires a Simplex, so it must not be offered for a domain=\"Real\" vector: " + distrNames); + // IID must still appear: it legitimately applies a scalar distribution + // independently to each element of any Real-domain vector, so the fix must not + // over-correct and hide it too. + assertTrue(distrNames.contains("IID"), + "IID legitimately applies to a domain=\"Real\" vector, so it should still be offered: " + distrNames); + } + + + @Test + public void simplexOffersDirichletButNotIID() throws Exception { + // Reverse/positive-control case: a true Simplex-valued param (elements in [0,1] + // that sum to 1) is exactly what Dirichlet requires, so it should be offered + // here. IID's default template targets a plain "Real" domain, not a Simplex, so + // it should be excluded -- this proves the fix actually distinguishes the two + // cases both ways, rather than just suppressing Dirichlet unconditionally. + // dimension 4, like DNA base frequencies (A, C, G, T) + SimplexParam freqs = new SimplexParam(new double[] {0.3, 0.2, 0.25, 0.25}); + + Set distrNames = distrNamesOfferedFor(freqs, "bFreqs.s:anolis"); + + assertTrue(distrNames.contains("Dirichlet"), + "Dirichlet should be offered for a Simplex-valued vector: " + distrNames); + assertFalse(distrNames.contains("IID"), + "IID's default template targets domain=\"Real\", not Simplex, so it should not be offered: " + distrNames); + } + + + /** + * Builds a single-item "distribution" list (as PriorListInputEditor does for the + * Priors panel), wraps {@code param} in an IID prior so there is a concrete + * TensorDistribution to attach the "distr" combo box editor to, creates that + * editor exactly the way BEAUti does, and returns the display names of the + * distributions offered in its combo box. + */ + @SuppressWarnings({"unchecked", "rawtypes"}) + private Set distrNamesOfferedFor(RealVectorParam param, String paramID) throws Exception { + AtomicReference error = new AtomicReference<>(); + AtomicReference> result = new AtomicReference<>(); + runOnFXThread(() -> { + BeautiDoc doc = new BeautiDoc(); + doc.loadTemplate(doc.processTemplate(BeautiConfig.TEMPLATE_DIR + "/Standard.xml")); + + param.setID(paramID); + + Normal normal = new Normal(); + normal.initByName( + "mean", new RealScalarParam<>(0.0, Real.INSTANCE), + "sigma", new RealScalarParam<>(1.0, PositiveReal.INSTANCE)); + + // wrap param in a valid prior so there is a TensorDistribution to hang the + // "distr" editor off, just like a row already in the Priors panel + IID iid = new IID(); + iid.initByName("param", param, "distr", normal); + iid.setID(paramID + ".prior"); + + doc.registerPlugin(param); + doc.registerPlugin(normal); + doc.registerPlugin(iid); + + List distributions = new ArrayList<>(List.of((Distribution) iid)); + Input> listInput = + new Input<>("distribution", "priors", distributions, Distribution.class); + + // this is what PriorListInputEditor.addPluginItem does for every row shown + // in the Priors panel + InputEditor editor = doc.getInputEditorFactory().createInputEditor( + listInput, 0, iid, false, InputEditor.ExpandOption.FALSE, + InputEditor.ButtonStatus.NONE, null, doc); + assertNotNull(editor, "no input editor created for the distr list item"); + + ComboBox comboBox = findComboBox(editor.getComponent()); + assertNotNull(comboBox, "no combo box to select a distribution from"); + + result.set(comboBox.getItems().stream() + .map(item -> ((BeautiSubTemplate) item).toString()) + .collect(Collectors.toSet())); + }, error); + + if (error.get() != null) { + error.get().printStackTrace(); + fail("test could not be run: " + error.get()); + } + return result.get(); + } + + + /** first combo box in the node tree rooted at node, or null if there is none **/ + private static ComboBox findComboBox(Node node) { + if (node instanceof ComboBox comboBox) { + return comboBox; + } + if (node instanceof Parent parent) { + for (Node child : parent.getChildrenUnmodifiable()) { + ComboBox comboBox = findComboBox(child); + if (comboBox != null) { + return comboBox; + } + } + } + return null; + } + + + /** run task on the JavaFX application thread, recording any exception in error **/ + private static void runOnFXThread(FXTask task, AtomicReference error) throws InterruptedException { + CountDownLatch latch = new CountDownLatch(1); + Platform.runLater(() -> { + try { + task.run(); + } catch (Throwable e) { + error.set(e); + } finally { + latch.countDown(); + } + }); + if (!latch.await(120, TimeUnit.SECONDS)) { + throw new AssertionError("timed out waiting for JavaFX thread"); + } + } + + private interface FXTask { + void run() throws Exception; + } +} From 808b9ed71a9f52b48a1ce47d918662a9a5b4ec4c Mon Sep 17 00:00:00 2001 From: walterxie Date: Mon, 31 Aug 2026 17:09:03 +1200 Subject: [PATCH 5/6] fix build and stop estimate in the Site Model panel from triggering a hyperprior #157 #162 --- .../app/inputeditor/spec/ScalarInputEditor.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) 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 d3029986..15ad4539 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 @@ -210,13 +210,16 @@ protected void addComboBox(Pane box, Input input, BEASTInterface beastObject) //m_editPluginButton.setVisible(false); //m_bAddButtons = false; if (itemNr < 0) { - for (Object beastObject2 : ((BEASTInterface) m_input.get()).getOutputs()) { - if (beastObject2 instanceof ScalarDistribution) { - m_isEstimatedBox.setVisible(doc.allowLinking); - m_isEstimatedBox.setVisible(true); - isParametricDistributionParameter = true; - break; - } + // reveal the checkbox (and allow a hyperprior) only for a distribution's + // own hyperparameters (e.g. Normal's mean/sigma), not its bound value + // ("param", e.g. mutationRate) -- see #157 + // this stops estimate in the Site Model panel from triggering a hyperprior + boolean isHyperparameter = beastObject instanceof ScalarDistribution + && !"param".equals(input.getName()); + if (isHyperparameter) { + m_isEstimatedBox.setVisible(doc.allowLinking); + m_isEstimatedBox.setVisible(true); + isParametricDistributionParameter = true; } for (Object beastObject2 : ((BEASTInterface) m_input.get()).getOutputs()) { if (beastObject2 instanceof Operator) { From 66340e3911dead4b0d5970b681ba201f9a429cbf Mon Sep 17 00:00:00 2001 From: walterxie Date: Thu, 3 Sep 2026 16:48:44 +1200 Subject: [PATCH 6/6] fix bugs #164 #165 #166, and complete #162 --- .../app/inputeditor/IIDInputEditor.java | 91 +++--- .../MarkovChainDistributionInputEditor.java | 35 +++ .../TensorDistributionInputEditor.java | 96 +++---- .../inputeditor/spec/ScalarInputEditor.java | 14 +- beast-fx/src/main/java/module-info.java | 3 +- .../fxtemplates/ParametricDistributions.xml | 2 +- .../beast.fx/fxtemplates/TreePriors.xml | 14 +- .../app/beauti/SimpleTreePriorTest.java | 17 +- .../TensorDistributionInputEditorTest.java | 259 +++++++++++++++--- .../spec/ScalarInputEditorTest.java | 132 +++++++++ beast-fx/version.xml | 1 + 11 files changed, 504 insertions(+), 160 deletions(-) create mode 100644 beast-fx/src/main/java/beastfx/app/inputeditor/MarkovChainDistributionInputEditor.java create mode 100644 beast-fx/src/test/java/test/beastfx/app/inputeditor/spec/ScalarInputEditorTest.java diff --git a/beast-fx/src/main/java/beastfx/app/inputeditor/IIDInputEditor.java b/beast-fx/src/main/java/beastfx/app/inputeditor/IIDInputEditor.java index 3412eb02..1ec0886e 100644 --- a/beast-fx/src/main/java/beastfx/app/inputeditor/IIDInputEditor.java +++ b/beast-fx/src/main/java/beastfx/app/inputeditor/IIDInputEditor.java @@ -5,15 +5,12 @@ import beast.base.core.Input; import beast.base.inference.Distribution; import beast.base.parser.PartitionContext; -import beast.base.spec.domain.*; +import beast.base.spec.domain.Bool; import beast.base.spec.inference.distribution.IID; import beast.base.spec.inference.distribution.ScalarDistribution; import beast.base.spec.inference.distribution.TensorDistribution; import beast.base.spec.inference.parameter.*; -import beast.base.spec.type.IntVector; -import beast.base.spec.type.RealVector; -import beast.base.spec.type.Scalar; -import beast.base.spec.type.Simplex; +import beast.base.spec.type.*; import beastfx.app.util.FXUtils; import javafx.geometry.Insets; import javafx.scene.Node; @@ -175,19 +172,50 @@ private Class getDomain(TensorDistribution value) { } + /** + * The per-element domain of a vector-valued {@code param} (e.g. {@code Real}, + * {@code PositiveReal}, {@code UnitInterval}), used by {@link #isCompatible} to + * decide whether a prior template's declared domain covers it. This is only the + * numeric range each element must fall in -- it says nothing about structural + * constraints across elements (e.g. a Simplex's "elements sum to 1"), which + * {@link #hasSimplexParam} checks separately and independently. + *

+ * {@code IIDInputEditor} is the dedicated editor registered for {@code IID.class}, + * so this only ever runs for IID rows. Every other {@link TensorDistribution} row + * with no dedicated editor of its own (currently just Dirichlet) instead goes through + * {@link TensorDistributionInputEditor}'s identical copy of this method. + * + * @param param the vector-valued input to a {@link TensorDistribution}, e.g. a + * {@code RealVectorParam} or {@code IntVectorParam} + * @return the {@link beast.base.spec.domain.Domain} subclass constraining param's + * elements, or {@code Scalar.class} if param isn't a recognised vector type + */ private Class getParameterDomain(Object param) { + // a real-valued state-node vector (RealVectorParam, SimplexParam, ...) carries its + // own declared domain (e.g. Real, PositiveReal, UnitInterval) as an input if (param instanceof RealVectorParam rvp) { return rvp.domainTypeInput.get().getClass(); } + // same, for an int-valued state-node vector (IntVectorParam, IntSimplexParam, ...) if (param instanceof IntVectorParam ivp) { return ivp.domainTypeInput.get().getClass(); } + // not a state-node param but still implements RealVector (e.g. a plain value, not + // backed by an Input) -- fall back to reading the domain off the type itself if (param instanceof RealVector rs) { return rs.getDomain().getClass(); } + // same fallback for a non-param IntVector if (param instanceof IntVector is) { return is.getDomain().getClass(); } + // boolean vectors have only one possible domain (true/false), so there is nothing + // to look up + if (param instanceof BoolVector) { + return Bool.class; + } + // param isn't any recognised vector type -- shouldn't happen for a vector-valued + // prior's "param" input, but don't guess a domain we can't back up return Scalar.class; } @@ -351,6 +379,14 @@ private boolean hasSimplexParam(Object param, TensorDistribution templateIn return paramIsSimplex == templateIsSimplex; } + /** + * Compatible when one domain's range of values fully covers the other's: the + * template's domain is broad enough for the param (e.g. IID's "Real" template + * offered for a PositiveReal-domain vector), or narrow enough to stay within it + * (e.g. a PositiveReal-only template offered for a NonNegativeReal param). Checked + * both ways since neither direction alone covers both cases. Unrelated families + * (Real vs. Int vs. Bool) satisfy neither direction and are correctly rejected. + */ private boolean isCompatible(Class paramDomain, Class templateDomain, boolean hasSimplexParam) { if (templateDomain == null) { // the "no prior" and ScalarDistribution templates should be rejected @@ -360,50 +396,7 @@ private boolean isCompatible(Class paramDomain, Class templateDomain, bool return false; } - if (Real.class.isAssignableFrom(paramDomain)) { - // check type first - if (!(Real.class.isAssignableFrom(templateDomain))) { - return false; - } - // more range checks here - if (paramDomain == Real.class) { - return true; - } - if (templateDomain == paramDomain) { - return true; - } - if (templateDomain == NonNegativeReal.class && paramDomain == PositiveReal.class) { - return true; - } - if (templateDomain == PositiveReal.class && paramDomain == NonNegativeReal.class) { - return true; - } - return false; - } - - if (Int.class.isAssignableFrom(paramDomain)) { - // check type first - if (!(Int.class.isAssignableFrom(templateDomain))) { - return false; - } - // more range checks here - if (paramDomain == Int.class) { - return true; - } - if (templateDomain == paramDomain) { - return true; - } - if (templateDomain == NonNegativeInt.class && paramDomain == PositiveInt.class) { - return true; - } - if (templateDomain == PositiveInt.class && paramDomain == NonNegativeInt.class) { - return true; - } - return false; - } - - // don't know how to handle -- err on the side of caution and accept anything - return true; + return templateDomain.isAssignableFrom(paramDomain) || paramDomain.isAssignableFrom(templateDomain); } diff --git a/beast-fx/src/main/java/beastfx/app/inputeditor/MarkovChainDistributionInputEditor.java b/beast-fx/src/main/java/beastfx/app/inputeditor/MarkovChainDistributionInputEditor.java new file mode 100644 index 00000000..775b33f9 --- /dev/null +++ b/beast-fx/src/main/java/beastfx/app/inputeditor/MarkovChainDistributionInputEditor.java @@ -0,0 +1,35 @@ +package beastfx.app.inputeditor; + + +import beast.base.spec.inference.distribution.MarkovChainDistribution; +import javafx.scene.control.ComboBox; + +/** + * Dedicated editor for a MarkovChainDistribution row in the Priors panel (e.g. Bayesian + * Skyline's "Markov chained prior on population sizes"). Unlike IID or Dirichlet, + * MarkovChainDistribution is not meant to be freely swapped for another prior via the + * generic "distr" combo box: it requires specific wiring (jeffreys/initialMean) that a + * domain-only compatibility check can't validate, and it has no registered subtemplate + * of its own to switch back to. So this editor never offers a picker at all, and (being + * registered for MarkovChainDistribution.class specifically, not TensorDistribution.class) + * it never appears as a candidate in any other row's picker either. + */ +public class MarkovChainDistributionInputEditor extends TensorDistributionInputEditor { + + public MarkovChainDistributionInputEditor() { + super(); + } + public MarkovChainDistributionInputEditor(BeautiDoc doc) { + super(doc); + } + + @Override + public Class type() { + return MarkovChainDistribution.class; + } + + @Override + protected ComboBox createComboBox() { + return null; + } +} diff --git a/beast-fx/src/main/java/beastfx/app/inputeditor/TensorDistributionInputEditor.java b/beast-fx/src/main/java/beastfx/app/inputeditor/TensorDistributionInputEditor.java index 22178875..c7c20dfa 100644 --- a/beast-fx/src/main/java/beastfx/app/inputeditor/TensorDistributionInputEditor.java +++ b/beast-fx/src/main/java/beastfx/app/inputeditor/TensorDistributionInputEditor.java @@ -5,14 +5,11 @@ import beast.base.core.Input; import beast.base.inference.Distribution; import beast.base.parser.PartitionContext; -import beast.base.spec.domain.*; +import beast.base.spec.domain.Bool; import beast.base.spec.inference.distribution.ScalarDistribution; import beast.base.spec.inference.distribution.TensorDistribution; import beast.base.spec.inference.parameter.*; -import beast.base.spec.type.IntVector; -import beast.base.spec.type.RealVector; -import beast.base.spec.type.Scalar; -import beast.base.spec.type.Simplex; +import beast.base.spec.type.*; import beastfx.app.util.FXUtils; import javafx.scene.Node; import javafx.scene.control.Button; @@ -53,7 +50,7 @@ public Class type() { @Override public void init(Input input, BEASTInterface beastObject, int itemNr, ExpandOption isExpandOption, boolean addButtons) { // useDefaultBehavior = !((beastObject instanceof beast.base.inference.distribution.Prior) || beastObject instanceof MRCAPrior || beastObject instanceof TreeDistribution); - + if (tensorTemplates == null) { Input> _input = new Input<>("param", "dummy input"); _input.setType(TensorDistribution.class); @@ -173,19 +170,51 @@ private Class getDomain(TensorDistribution value) { } + /** + * The per-element domain of a vector-valued {@code param} (e.g. {@code Real}, + * {@code PositiveReal}, {@code UnitInterval}), used by {@link #isCompatible} to + * decide whether a prior template's declared domain covers it. This is only the + * numeric range each element must fall in -- it says nothing about structural + * constraints across elements (e.g. a Simplex's "elements sum to 1"), which + * {@link #hasSimplexParam} checks separately and independently. + *

+ * This editor is the fallback used for any {@link TensorDistribution} row whose + * concrete class has no dedicated editor of its own -- currently Dirichlet, and any + * future spec vector distribution added without one. IID has its own dedicated + * {@link IIDInputEditor}, which carries an identical copy of this method for IID rows + * specifically. + * + * @param param the vector-valued input to a {@link TensorDistribution}, e.g. a + * {@code RealVectorParam} or {@code IntVectorParam} + * @return the {@link beast.base.spec.domain.Domain} subclass constraining param's + * elements, or {@code Scalar.class} if param isn't a recognised vector type + */ private Class getParameterDomain(Object param) { + // a real-valued state-node vector (RealVectorParam, SimplexParam, ...) carries its + // own declared domain (e.g. Real, PositiveReal, UnitInterval) as an input if (param instanceof RealVectorParam rvp) { return rvp.domainTypeInput.get().getClass(); } + // same, for an int-valued state-node vector (IntVectorParam, IntSimplexParam, ...) if (param instanceof IntVectorParam ivp) { return ivp.domainTypeInput.get().getClass(); } + // not a state-node param but still implements RealVector (e.g. a plain value, not + // backed by an Input) -- fall back to reading the domain off the type itself if (param instanceof RealVector rs) { return rs.getDomain().getClass(); } + // same fallback for a non-param IntVector if (param instanceof IntVector is) { return is.getDomain().getClass(); } + // boolean vectors have only one possible domain (true/false), so there is nothing + // to look up + if (param instanceof BoolVector) { + return Bool.class; + } + // param isn't any recognised vector type -- shouldn't happen for a vector-valued + // prior's "param" input, but don't guess a domain we can't back up return Scalar.class; } @@ -245,7 +274,7 @@ protected void addInputLabel() { private ComboBox comboBox; - private ComboBox createComboBox() { + protected ComboBox createComboBox() { ComboBox comboBox = new ComboBox<>(); TensorDistribution prior = (TensorDistribution) m_beastObject; @@ -350,6 +379,14 @@ private boolean hasSimplexParam(Object param, TensorDistribution templateIn return paramIsSimplex == templateIsSimplex; } + /** + * Compatible when one domain's range of values fully covers the other's: the + * template's domain is broad enough for the param (e.g. IID's "Real" template + * offered for a PositiveReal-domain vector), or narrow enough to stay within it + * (e.g. a PositiveReal-only template offered for a NonNegativeReal param). Checked + * both ways since neither direction alone covers both cases. Unrelated families + * (Real vs. Int vs. Bool) satisfy neither direction and are correctly rejected. + */ private boolean isCompatible(Class paramDomain, Class templateDomain, boolean hasSimplexParam) { if (templateDomain == null) { // the "no prior" and ScalarDistribution templates should be rejected @@ -359,50 +396,7 @@ private boolean isCompatible(Class paramDomain, Class templateDomain, bool return false; } - if (Real.class.isAssignableFrom(paramDomain)) { - // check type first - if (!(Real.class.isAssignableFrom(templateDomain))) { - return false; - } - // more range checks here - if (paramDomain == Real.class) { - return true; - } - if (templateDomain == paramDomain) { - return true; - } - if (templateDomain == NonNegativeReal.class && paramDomain == PositiveReal.class) { - return true; - } - if (templateDomain == PositiveReal.class && paramDomain == NonNegativeReal.class) { - return true; - } - return false; - } - - if (Int.class.isAssignableFrom(paramDomain)) { - // check type first - if (!(Int.class.isAssignableFrom(templateDomain))) { - return false; - } - // more range checks here - if (paramDomain == Int.class) { - return true; - } - if (templateDomain == paramDomain) { - return true; - } - if (templateDomain == NonNegativeInt.class && paramDomain == PositiveInt.class) { - return true; - } - if (templateDomain == PositiveInt.class && paramDomain == NonNegativeInt.class) { - return true; - } - return false; - } - - // don't know how to handle -- err on the side of caution and accept anything - return true; + return templateDomain.isAssignableFrom(paramDomain) || paramDomain.isAssignableFrom(templateDomain); } 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 15ad4539..df09487d 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 @@ -210,12 +210,19 @@ protected void addComboBox(Pane box, Input input, BEASTInterface beastObject) //m_editPluginButton.setVisible(false); //m_bAddButtons = false; if (itemNr < 0) { + String inputName = input.getName(); + // "lower"/"upper" (e.g. Uniform, LogUniform, BoundedReal/BoundedInt) define + // the support of the distribution itself, not a value to be sampled -- there + // is no sensible prior/operator for "the bound of my own bound" + boolean isDistributionBound = beastObject instanceof ScalarDistribution + && ("lower".equals(inputName) || "upper".equals(inputName)); // reveal the checkbox (and allow a hyperprior) only for a distribution's // own hyperparameters (e.g. Normal's mean/sigma), not its bound value // ("param", e.g. mutationRate) -- see #157 // this stops estimate in the Site Model panel from triggering a hyperprior boolean isHyperparameter = beastObject instanceof ScalarDistribution - && !"param".equals(input.getName()); + && !"param".equals(inputName) + && !isDistributionBound; if (isHyperparameter) { m_isEstimatedBox.setVisible(doc.allowLinking); m_isEstimatedBox.setVisible(true); @@ -228,6 +235,11 @@ protected void addComboBox(Pane box, Input input, BEASTInterface beastObject) break; } } + if (isDistributionBound) { + // never offer to estimate a distribution's own bound, regardless of + // expert mode or an operator that happens to reference it + m_isEstimatedBox.setVisible(false); + } } else { for (Object beastObject2 : ((BEASTInterface) ((List)m_input.get()).get(itemNr)).getOutputs()) { if (beastObject2 instanceof Operator) { diff --git a/beast-fx/src/main/java/module-info.java b/beast-fx/src/main/java/module-info.java index d70bb01d..b9a4e8cf 100644 --- a/beast-fx/src/main/java/module-info.java +++ b/beast-fx/src/main/java/module-info.java @@ -111,7 +111,8 @@ beastfx.app.inputeditor.spec.VectorInputEditor, beastfx.app.inputeditor.ScalarDistributionInputEditor, beastfx.app.inputeditor.TensorDistributionInputEditor, - beastfx.app.inputeditor.IIDInputEditor; + beastfx.app.inputeditor.IIDInputEditor, + beastfx.app.inputeditor.MarkovChainDistributionInputEditor; provides beastfx.app.inputeditor.AlignmentImporter with beastfx.app.inputeditor.NexusImporter, diff --git a/beast-fx/src/main/resources/beast.fx/fxtemplates/ParametricDistributions.xml b/beast-fx/src/main/resources/beast.fx/fxtemplates/ParametricDistributions.xml index bebbdaba..14b8dc7a 100644 --- a/beast-fx/src/main/resources/beast.fx/fxtemplates/ParametricDistributions.xml +++ b/beast-fx/src/main/resources/beast.fx/fxtemplates/ParametricDistributions.xml @@ -238,7 +238,7 @@ - + ]]> diff --git a/beast-fx/src/main/resources/beast.fx/fxtemplates/TreePriors.xml b/beast-fx/src/main/resources/beast.fx/fxtemplates/TreePriors.xml index b18c36d9..17ac1057 100644 --- a/beast-fx/src/main/resources/beast.fx/fxtemplates/TreePriors.xml +++ b/beast-fx/src/main/resources/beast.fx/fxtemplates/TreePriors.xml @@ -460,21 +460,19 @@ EBSPLogger/mode/ - + - - - - - - + + + + - + diff --git a/beast-fx/src/test/java/test/beastfx/app/beauti/SimpleTreePriorTest.java b/beast-fx/src/test/java/test/beastfx/app/beauti/SimpleTreePriorTest.java index b47dd531..651d35fd 100644 --- a/beast-fx/src/test/java/test/beastfx/app/beauti/SimpleTreePriorTest.java +++ b/beast-fx/src/test/java/test/beastfx/app/beauti/SimpleTreePriorTest.java @@ -1,15 +1,14 @@ package test.beastfx.app.beauti; -import java.io.File; - +import beastfx.app.beauti.BeautiTabPane; +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 beastfx.app.beauti.BeautiTabPane; -import javafx.stage.Stage; +import java.io.File; @ExtendWith(ApplicationExtension.class) public class SimpleTreePriorTest extends BeautiBase { @@ -57,7 +56,15 @@ public void simpleTreePriorTest(FxRobot robot) throws Exception { assertOperatorsEqual("BayesianSkylineTreeRootScaler.t:anolis", "BayesianSkylineUniformOperator.t:anolis", "BayesianSkylineSubtreeSlide.t:anolis", "BayesianSkylineNarrow.t:anolis", "BayesianSkylineWide.t:anolis", "BayesianSkylineWilsonBalding.t:anolis", "BayesianSkylineTreeScaler.t:anolis", "popSizesScaler.t:anolis", "groupSizesDelta.t:anolis"); assertPriorsEqual("BayesianSkyline.t:anolis", "MarkovChainedPopSizes.t:anolis"); assertTraceLogEqual("posterior", "likelihood", "prior", "treeLikelihood.anolis", "TreeHeight.t:anolis", "BayesianSkyline.t:anolis", "bPopSizes.t:anolis", "bGroupSizes.t:anolis"); - + + warning("Change to Coalescent - Extended BSP"); + robot.clickOn("#TreeDistribution").clickOn("Coalescent Extended Bayesian Skyline"); + printBeautiState(); + assertStateEquals("Tree.t:anolis", "indicators.alltrees", "populationMean.alltrees", "popSizes.alltrees"); + assertOperatorsEqual("ExtendedBayesianSkylineTreeScaler.t:anolis", "ExtendedBayesianSkylineTreeRootScaler.t:anolis", "ExtendedBayesianSkylineUniformOperator.t:anolis", "ExtendedBayesianSkylineSubtreeSlide.t:anolis", "ExtendedBayesianSkylineNarrow.t:anolis", "ExtendedBayesianSkylineWide.t:anolis", "ExtendedBayesianSkylineWilsonBalding.t:anolis", "bitflip.alltrees", "indicatorSampler.alltrees", "indicatorScaler.alltrees", "EBSPupDownOperator.alltrees"); + assertPriorsEqual("ExtendedBayesianSkyline.t:anolis", "populationMeanPrior.alltrees", "indicatorsPrior.alltrees", "popSizePrior.alltrees"); + assertTraceLogEqual("posterior", "likelihood", "prior", "treeLikelihood.anolis", "TreeHeight.t:anolis", "ExtendedBayesianSkyline.t:anolis", "indicators.alltrees", "populationMean.alltrees", "popSizes.alltrees", "sumIndicators"); + warning("Change to Yule"); robot.clickOn("#TreeDistribution").clickOn("Yule Model"); printBeautiState(); diff --git a/beast-fx/src/test/java/test/beastfx/app/inputeditor/TensorDistributionInputEditorTest.java b/beast-fx/src/test/java/test/beastfx/app/inputeditor/TensorDistributionInputEditorTest.java index 8b3513f7..961e64e4 100644 --- a/beast-fx/src/test/java/test/beastfx/app/inputeditor/TensorDistributionInputEditorTest.java +++ b/beast-fx/src/test/java/test/beastfx/app/inputeditor/TensorDistributionInputEditorTest.java @@ -1,15 +1,12 @@ package test.beastfx.app.inputeditor; +import beast.base.core.BEASTInterface; import beast.base.core.Input; import beast.base.inference.Distribution; -import beast.base.spec.domain.PositiveReal; -import beast.base.spec.domain.Real; -import beast.base.spec.inference.distribution.IID; -import beast.base.spec.inference.distribution.Normal; -import beast.base.spec.inference.parameter.RealScalarParam; -import beast.base.spec.inference.parameter.RealVectorParam; -import beast.base.spec.inference.parameter.SimplexParam; +import beast.base.spec.domain.*; +import beast.base.spec.inference.distribution.*; +import beast.base.spec.inference.parameter.*; import beastfx.app.inputeditor.*; import javafx.application.Platform; import javafx.scene.Node; @@ -36,13 +33,25 @@ /** - * Regression test for issue #160: a vector-valued parameter declared with an - * unconstrained {@code domain="Real"} -- e.g. Bayesian Skyline's population sizes, - * {@code } -- must not offer - * Dirichlet in the Priors panel's "distr" combo box, since Dirichlet only applies to - * a Simplex (elements constrained to [0,1] and summing to 1), not an arbitrary real - * vector. IID legitimately applies to a Real-domain vector, so it must still be - * offered there; conversely it must not be offered for a Simplex-valued parameter. + * Regression tests for the "distr" combo box in the Priors panel (issue #160, and the + * domain-inherited-from-Real / Boolean-vector bugs found alongside it): + *

    + *
  • a vector param declared with the generic, unconstrained {@code domain="Real"} -- + * e.g. Bayesian Skyline's popSizes -- must offer IID but not Dirichlet;
    • + *
  • a Simplex-valued param (elements in [0,1] summing to 1) must offer Dirichlet but + * not IID;
    • + *
  • a vector param declared with a domain narrower than Real (e.g. PositiveReal) must + * still offer IID -- IID's own template declares the unconstrained "Real" domain, so + * the compatibility check must recognise that an unconstrained template covers every + * narrower Real-family domain, not just an exact {@code domain="Real"} match;
    • + *
  • a Boolean vector param must not be offered every template indiscriminately -- + * {@code getParameterDomain} previously had no case for {@code BoolVector}, so it fell + * through to a sentinel the compatibility check also didn't recognise, and treated as + * "compatible with anything".
    • + *
+ * Note: the editor actually behind an IID row is {@link IIDInputEditor}, not the more + * generic {@link TensorDistributionInputEditor} (used for other TensorDistribution rows, + * e.g. Dirichlet, that have no dedicated editor) -- both classes had the same bug. */ @ExtendWith(ApplicationExtension.class) public class TensorDistributionInputEditorTest { @@ -52,14 +61,16 @@ public void start(Stage stage) { // nothing to show, only the JavaFX toolkit is required } - /** the editor caches templates in static fields, so do not leak them into other tests **/ + /** both editors cache templates in static fields, so do not leak them into other tests **/ @BeforeEach @AfterEach public void clearTemplateCache() throws Exception { - for (String name : new String[] {"tensorTemplates", "templateInstances", "templateDomains"}) { - Field field = TensorDistributionInputEditor.class.getDeclaredField(name); - field.setAccessible(true); - field.set(null, null); + for (Class editorClass : new Class[] {TensorDistributionInputEditor.class, IIDInputEditor.class}) { + for (String name : new String[] {"tensorTemplates", "templateInstances", "templateDomains"}) { + Field field = editorClass.getDeclaredField(name); + field.setAccessible(true); + field.set(null, null); + } } } @@ -89,6 +100,75 @@ public void realDomainVectorDoesNotOfferDirichletButOffersIID() throws Exception } + @Test + public void positiveRealDomainVectorOffersIID() throws Exception { + // a domain narrower than the generic "Real" (PositiveReal "inherits" from Real) + // must still offer IID: IID's own template declares domain="Real", and the + // compatibility check must accept an unconstrained template for any narrower + // Real-family param, not just an exact domain match. + RealVectorParam rates = new RealVectorParam<>( + new double[] {0.5, 0.5}, PositiveReal.INSTANCE); + + Set distrNames = distrNamesOfferedFor(rates, "rates.s:anolis"); + + assertTrue(distrNames.contains("IID"), + "IID should still be offered for a PositiveReal (Real-family) vector: " + distrNames); + } + + + @Test + public void intVectorOffersNoCompatiblePriorYet() throws Exception { + // IID's only registered template targets domain="Real"; there is no Int-domain + // IID template yet, so an Int vector currently has no compatible entry in the + // "distr" combo box at all. This documents the current (safe) behaviour -- + // registering an Int-domain IID template would need createComboBox()'s "restore + // selected template" match-by-class-name loop to also disambiguate by domain + // first, since it would then share IID's class name with the Real-domain one. + IntVectorParam counts = new IntVectorParam<>(new int[] {1, 2}, NonNegativeInt.INSTANCE); + counts.setID("counts.s:anolis"); + + IntUniform intUniform = new IntUniform( + new IntScalarParam(0, Int.INSTANCE), + new IntScalarParam(0, Int.INSTANCE), + new IntScalarParam(10, Int.INSTANCE)); + + IID iid = new IID(); + iid.initByName("param", counts, "distr", intUniform); + iid.setID("counts.prior"); + + Set distrNames = distrNamesOfferedFor(iid, counts, intUniform); + + assertTrue(distrNames.isEmpty(), + "no Int-domain prior template is registered yet, so none should be offered for an Int vector: " + distrNames); + } + + + @Test + public void boolVectorDoesNotOfferEveryTemplate() throws Exception { + // getParameterDomain() had no case for BoolVector, so it fell back to a domain + // class the compatibility check also didn't recognise, and that fell through to + // "don't know how to handle -- accept anything", offering every real/int-typed + // prior (including IID) for a Boolean vector. There is no Boolean-domain prior + // template registered yet, so the fix must show none, not all of them. + BoolVectorParam flags = new BoolVectorParam(new boolean[] {true, false}); + flags.setID("flags.s:anolis"); + + Bernoulli bernoulli = new Bernoulli(); + bernoulli.initByName( + "param", new BoolScalarParam(false), + "p", new RealScalarParam<>(0.5, UnitInterval.INSTANCE)); + + IID iid = new IID(); + iid.initByName("param", flags, "distr", bernoulli); + iid.setID("flags.prior"); + + Set distrNames = distrNamesOfferedFor(iid, flags, bernoulli); + + assertTrue(distrNames.isEmpty(), + "no prior template declares a Boolean domain yet, so none should be offered for a Boolean vector: " + distrNames); + } + + @Test public void simplexOffersDirichletButNotIID() throws Exception { // Reverse/positive-control case: a true Simplex-valued param (elements in [0,1] @@ -108,53 +188,144 @@ public void simplexOffersDirichletButNotIID() throws Exception { } + @Test + public void markovChainDistributionRowOffersNoDistrPicker() throws Exception { + // Bayesian Skyline wraps its popSizes (a PositiveReal vector) in a + // MarkovChainDistribution ("Markov chained prior on population sizes"), connected + // to the same "distribution" list the Priors panel renders -- see + // TreePriors.xml's CoalescentBayesianSkyline subtemplate. MarkovChainDistribution + // requires specific wiring (jeffreys/initialMean) that a domain-only compatibility + // check can't validate, and has no registered subtemplate to switch back to, so + // swapping it via a generic "distr" combo box (which would offer IID, since IID's + // "Real" template covers any Real-family domain including PositiveReal) would be + // unsafe and irreversible. MarkovChainDistributionInputEditor -- registered for + // MarkovChainDistribution.class specifically, taking priority over the generic + // TensorDistributionInputEditor -- fixes this by never offering a picker at all, + // without adding any MarkovChainDistribution-specific logic to the generic editor. + RealVectorParam popSizes = new RealVectorParam<>( + new double[] {380.0, 380.0, 380.0, 380.0, 380.0}, PositiveReal.INSTANCE); + popSizes.setID("bPopSizes.t:anolis"); + + MarkovChainDistribution mcd = new MarkovChainDistribution(); + mcd.initByName("param", popSizes, "jeffreys", true); + mcd.setID("MarkovChainedPopSizes.t:anolis"); + + Set distrNames = distrNamesOfferedFor(mcd, popSizes, null); + + assertTrue(distrNames.isEmpty(), + "MarkovChainDistributionInputEditor should never offer a distr picker: " + distrNames); + } + + + @Test + public void markovChainDistributionNeverOffersItselfAsAlternative() throws Exception { + // MarkovChainDistribution must not appear as a swap-to option in some other, + // unrelated PositiveReal vector's row either (unlike IID, which legitimately + // applies to any such vector) -- it has no registered subtemplate at all, so it + // can never be scanned as a candidate regardless of domain compatibility. + RealVectorParam rates = new RealVectorParam<>( + new double[] {0.5, 0.5}, PositiveReal.INSTANCE); + + Set distrNames = distrNamesOfferedFor(rates, "rates.s:anolis"); + + assertFalse(distrNames.contains("Markov Chain Distribution"), + "MarkovChainDistribution has no registered subtemplate, so it must never be offered as an alternative for an unrelated vector: " + distrNames); + } + + + @Test + public void intSumWiresIntoNewSpecPoissonWithoutPrior() throws Exception { + // EBSP's "indicatorsPrior.alltrees" needs a Poisson prior on a *computed* count + // (how many of indicators.alltrees are true), not on a StateNode directly. The + // deprecated beast.base.spec.inference.distribution.Prior wrapper used to do this, + // but Prior is unusable (its own "distr" input is typed to the legacy + // ParametricDistribution hierarchy, incompatible with every spec-package + // distribution) and must not be used. IntSum -- unlike Sum, which always exposes a + // RealScalar -- exposes an IntScalar, so it wires directly into the spec-package + // Poisson's "param" input (IntScalar) with no wrapper at all: + // Poisson itself, being a TensorDistribution, is a Distribution and can go straight + // into the "distribution" list. + BoolVectorParam indicators = new BoolVectorParam(new boolean[] {true, false, true}); + indicators.setID("indicators.alltrees"); + + beast.base.spec.evolution.IntSum indsSum = new beast.base.spec.evolution.IntSum(); + indsSum.initByName("arg", List.of(indicators)); + indsSum.setID("indsSum.alltrees"); + + beast.base.spec.inference.distribution.Poisson poisson = + new beast.base.spec.inference.distribution.Poisson(); + poisson.initByName("param", indsSum, + "lambda", new RealScalarParam<>(0.69314718056, PositiveReal.INSTANCE)); + poisson.setID("indicatorsPrior.alltrees"); + + assertEquals(2, indsSum.get(), "IntSum should count the 2 true indicators"); + assertTrue(Double.isFinite(poisson.calculateLogP()), + "Poisson.calculateLogP() should compute a finite logP directly from IntSum, with no Prior wrapper"); + } + + /** - * Builds a single-item "distribution" list (as PriorListInputEditor does for the - * Priors panel), wraps {@code param} in an IID prior so there is a concrete - * TensorDistribution to attach the "distr" combo box editor to, creates that - * editor exactly the way BEAUti does, and returns the display names of the - * distributions offered in its combo box. + * Convenience overload for the common case: wraps {@code param} in a fresh IID(Normal) + * prior, just like {@link #distrNamesOfferedFor(TensorDistribution, BEASTInterface, + * BEASTInterface)} but without the caller having to build that prior by hand. */ @SuppressWarnings({"unchecked", "rawtypes"}) private Set distrNamesOfferedFor(RealVectorParam param, String paramID) throws Exception { + param.setID(paramID); + + Normal normal = new Normal(); + normal.initByName( + "mean", new RealScalarParam<>(0.0, Real.INSTANCE), + "sigma", new RealScalarParam<>(1.0, PositiveReal.INSTANCE)); + + // wrap param in a valid prior so there is a TensorDistribution to hang the + // "distr" editor off, just like a row already in the Priors panel + IID iid = new IID(); + iid.initByName("param", param, "distr", normal); + iid.setID(paramID + ".prior"); + + return distrNamesOfferedFor(iid, param, normal); + } + + + /** + * Builds a single-item "distribution" list (as PriorListInputEditor does for the + * Priors panel) around the already-initialised {@code prior} (with IDs already set + * on {@code prior}, {@code param} and {@code distr}), creates the "distr" combo box + * editor for it exactly the way BEAUti does, and returns the display names of the + * distributions offered in its combo box (empty if none are compatible -- BEAUti + * shows no combo box at all in that case, rather than an empty one). {@code distr} + * may be {@code null} for a TensorDistribution with no separate nested distribution + * object of its own (e.g. MarkovChainDistribution, unlike IID/Dirichlet's "distr"/ + * "alpha"). + */ + @SuppressWarnings({"unchecked", "rawtypes"}) + private Set distrNamesOfferedFor(TensorDistribution prior, BEASTInterface param, BEASTInterface distr) throws Exception { AtomicReference error = new AtomicReference<>(); AtomicReference> result = new AtomicReference<>(); runOnFXThread(() -> { BeautiDoc doc = new BeautiDoc(); doc.loadTemplate(doc.processTemplate(BeautiConfig.TEMPLATE_DIR + "/Standard.xml")); - param.setID(paramID); - - Normal normal = new Normal(); - normal.initByName( - "mean", new RealScalarParam<>(0.0, Real.INSTANCE), - "sigma", new RealScalarParam<>(1.0, PositiveReal.INSTANCE)); - - // wrap param in a valid prior so there is a TensorDistribution to hang the - // "distr" editor off, just like a row already in the Priors panel - IID iid = new IID(); - iid.initByName("param", param, "distr", normal); - iid.setID(paramID + ".prior"); - doc.registerPlugin(param); - doc.registerPlugin(normal); - doc.registerPlugin(iid); + if (distr != null) { + doc.registerPlugin(distr); + } + doc.registerPlugin((BEASTInterface) prior); - List distributions = new ArrayList<>(List.of((Distribution) iid)); + List distributions = new ArrayList<>(List.of((Distribution) prior)); Input> listInput = new Input<>("distribution", "priors", distributions, Distribution.class); // this is what PriorListInputEditor.addPluginItem does for every row shown // in the Priors panel InputEditor editor = doc.getInputEditorFactory().createInputEditor( - listInput, 0, iid, false, InputEditor.ExpandOption.FALSE, + listInput, 0, (BEASTInterface) prior, false, InputEditor.ExpandOption.FALSE, InputEditor.ButtonStatus.NONE, null, doc); assertNotNull(editor, "no input editor created for the distr list item"); ComboBox comboBox = findComboBox(editor.getComponent()); - assertNotNull(comboBox, "no combo box to select a distribution from"); - - result.set(comboBox.getItems().stream() + result.set(comboBox == null ? Set.of() : comboBox.getItems().stream() .map(item -> ((BeautiSubTemplate) item).toString()) .collect(Collectors.toSet())); }, error); diff --git a/beast-fx/src/test/java/test/beastfx/app/inputeditor/spec/ScalarInputEditorTest.java b/beast-fx/src/test/java/test/beastfx/app/inputeditor/spec/ScalarInputEditorTest.java new file mode 100644 index 00000000..b7852f34 --- /dev/null +++ b/beast-fx/src/test/java/test/beastfx/app/inputeditor/spec/ScalarInputEditorTest.java @@ -0,0 +1,132 @@ +package test.beastfx.app.inputeditor.spec; + + +import beast.base.core.BEASTInterface; +import beast.base.core.Input; +import beast.base.spec.domain.Real; +import beast.base.spec.inference.distribution.Uniform; +import beast.base.spec.inference.parameter.RealScalarParam; +import beastfx.app.inputeditor.BeautiConfig; +import beastfx.app.inputeditor.BeautiDoc; +import beastfx.app.inputeditor.InputEditor; +import javafx.application.Platform; +import javafx.scene.Node; +import javafx.scene.Parent; +import javafx.scene.control.CheckBox; +import javafx.stage.Stage; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.testfx.framework.junit5.ApplicationExtension; +import org.testfx.framework.junit5.Start; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.jupiter.api.Assertions.*; + + +/** + * Regression test: a distribution's own "lower"/"upper" bound (e.g. Uniform's support) + * defines the distribution itself, not a value to be sampled -- there is no sensible + * prior/operator for "the bound of my own bound", so the "estimate" checkbox must never + * be offered for it, unlike a genuine hyperparameter (e.g. Normal's mean/sigma). + */ +@ExtendWith(ApplicationExtension.class) +public class ScalarInputEditorTest { + + @Start + public void start(Stage stage) { + // nothing to show, only the JavaFX toolkit is required + } + + @Test + public void uniformLowerAndUpperNeverOfferEstimate() throws Exception { + AtomicReference error = new AtomicReference<>(); + AtomicReference lowerVisible = new AtomicReference<>(); + AtomicReference upperVisible = new AtomicReference<>(); + runOnFXThread(() -> { + BeautiDoc doc = new BeautiDoc(); + doc.loadTemplate(doc.processTemplate(BeautiConfig.TEMPLATE_DIR + "/Standard.xml")); + + RealScalarParam lower = new RealScalarParam<>(-1e307, Real.INSTANCE); + lower.setID("lower.s:test"); + RealScalarParam upper = new RealScalarParam<>(1e307, Real.INSTANCE); + upper.setID("upper.s:test"); + + Uniform uniform = new Uniform(); + uniform.initByName("lower", lower, "upper", upper); + uniform.setID("uniformDist.s:test"); + + doc.registerPlugin(lower); + doc.registerPlugin(upper); + doc.registerPlugin(uniform); + + lowerVisible.set(isEstimateCheckboxVisible(doc, uniform, "lower")); + upperVisible.set(isEstimateCheckboxVisible(doc, uniform, "upper")); + }, error); + + if (error.get() != null) { + error.get().printStackTrace(); + fail("test could not be run: " + error.get()); + } + + assertFalse(lowerVisible.get(), "the estimate checkbox for Uniform's own \"lower\" bound must never be shown"); + assertFalse(upperVisible.get(), "the estimate checkbox for Uniform's own \"upper\" bound must never be shown"); + } + + + /** + * Builds the editor exactly as BEAUti does for a single (non-list) Input, the way an + * expanded distribution's own hyperparameter row is rendered, and reports whether its + * "estimate" checkbox ends up visible. + */ + private boolean isEstimateCheckboxVisible(BeautiDoc doc, BEASTInterface beastObject, String inputName) throws Exception { + Input input = beastObject.getInput(inputName); + InputEditor editor = doc.getInputEditorFactory().createInputEditor(input, beastObject, doc); + assertNotNull(editor, "no input editor created for " + inputName); + + CheckBox checkBox = findCheckBox(editor.getComponent(), inputName + ".isEstimated"); + assertNotNull(checkBox, "no \"" + inputName + ".isEstimated\" checkbox found"); + return checkBox.isVisible(); + } + + + /** the CheckBox with the given id in the node tree rooted at node, or null if there is none **/ + private static CheckBox findCheckBox(Node node, String id) { + if (node instanceof CheckBox checkBox && id.equals(checkBox.getId())) { + return checkBox; + } + if (node instanceof Parent parent) { + for (Node child : parent.getChildrenUnmodifiable()) { + CheckBox checkBox = findCheckBox(child, id); + if (checkBox != null) { + return checkBox; + } + } + } + return null; + } + + + /** run task on the JavaFX application thread, recording any exception in error **/ + private static void runOnFXThread(FXTask task, AtomicReference error) throws InterruptedException { + CountDownLatch latch = new CountDownLatch(1); + Platform.runLater(() -> { + try { + task.run(); + } catch (Throwable e) { + error.set(e); + } finally { + latch.countDown(); + } + }); + if (!latch.await(120, TimeUnit.SECONDS)) { + throw new AssertionError("timed out waiting for JavaFX thread"); + } + } + + private interface FXTask { + void run() throws Exception; + } +} diff --git a/beast-fx/version.xml b/beast-fx/version.xml index 4f51281a..bbb075eb 100644 --- a/beast-fx/version.xml +++ b/beast-fx/version.xml @@ -94,6 +94,7 @@ +