Skip to content

Fix several simple bugs in Prior panel - #161

Open
walterxie wants to merge 6 commits into
masterfrom
priorpanel
Open

Fix several simple bugs in Prior panel#161
walterxie wants to merge 6 commits into
masterfrom
priorpanel

Conversation

@walterxie

Copy link
Copy Markdown
Member

#159 — OneOnX prior removed in beast3, BEAUti had no replacement

Fix (beba5a4, c6093be):

  • Removed the leftover OneOnX template entry.
  • Added a LogUniform template instead — a proper, scale-invariant prior bounded on [1e-8, 1e8] for PositiveReal parameters, replacing the improper 1/x prior.
  • Tidied the explanatory comment above it.

#160 — Prior panel showing wrong info for parameters

Two separate bugs under this issue:

Bug 1 — Dirichlet prior parameter not shown (d7766f6):
The Dirichlet template was missing its param (simplex) input and suppressInputs setting, so BEAUti displayed it incorrectly in the Priors panel.
Fix: Added the missing SimplexParam and suppressInputs to the template.

Bug 2 — Wrong "type" label in the initial-value dialog (ebb69b9):
When you click a parameter's value/range button in the Priors panel, BEAUti opens a dialog whose title label is built from a Class argument. That argument was hardcoded to a generic interface (RealScalar.class, IntScalar.class) instead of the parameter's real class — so every dialog showed the same generic label (e.g. "RealScalar:") regardless of what parameter you were actually editing, and in two cases (IIDInputEditor, TensorDistributionInputEditor) it even used the scalar interface for vector parameters.
Fix: Pass p1.getClass() (the parameter's actual class) instead, in ScalarDistributionInputEditor, IIDInputEditor, and TensorDistributionInputEditor — 5 call sites total. Removed now-unused imports.

@walterxie
walterxie requested a review from alexeid August 26, 2026 22:57
@alexeid

alexeid commented Aug 27, 2026

Copy link
Copy Markdown
Member

EDIT: Sorry, my initial concern was incorrect. For a SimplexParam, Dirichlet is the only compatible entry, so the combo value never changes and the swap handler never runs. The problem only shows up for a vector parameter with domain="Real", where isCompatible takes the paramDomain == Real.class → true shortcut and offers Dirichlet alongside IID when it shouldn't. A related problem: no tensor template matches a PositiveReal vector parameter, so such rows get no selector at all. Is that by design?

IID is domain-polymorphic. What it can be applied to depends on its inner scalar distribution, not on its own param. But getDomain reads the domain off the template instance's placeholder param, which for IID is hardcoded domain="Real", so IID advertises itself as Real-only. That's the same placeholder mechanism this PR extends to Dirichlet. It is fine for Dirichlet, which genuinely is simplex-only, but it's the reason PositiveReal vectors match nothing. The fix is to special-case the two: IID compatible with any vector param (constrain the inner distr by the param's domain instead), Dirichlet only with a Simplex. That also closes the Real-vector hole from the earlier discussion, since Dirichlet would stop being offered there.

@alexeid

alexeid commented Aug 27, 2026

Copy link
Copy Markdown
Member

Small issues from the same pass.

In this PR

  1. The placeholder param works around a beast-base bug. TensorDistribution.paramInput is Validate.OPTIONAL, but dimension() returns 0 when it is absent (TensorDistribution.java:74), and Dirichlet.initAndValidate then compares that 0 against alpha.size() and throws. So a distribution blows up on an input declared optional. Fixing the check to skip when param == null may be preferable to every template shipping a dummy dim-1 simplex that has to be kept dimension-consistent by hand.
  2. The hmc attribute on the LogUniform subtemplate points at ParametricDistributions/LogUniform/{lower,upper}/, which almost certainly 404s on beast2-dev.github.io/hmc. Worth checking, or dropping the attribute until the pages exist.
  3. [1e-8, 1e8] are magic numbers. A line in the subtemplate comment explaining why those bounds would help future readers.
  4. Import churn runs in opposite directions in the same PR: IIDInputEditor goes explicit -> wildcard, TensorDistributionInputEditor goes wildcard -> explicit. Cosmetic, but it multiplies the apparent size of a 5-line fix.
  5. TensorDistributionInputEditor.java:102,123 and the equivalents in IIDInputEditor dereference prior1.paramInput.get() with no null guard, where ScalarDistributionInputEditor.java:164,187 has one. Pre-existing and it would NPE either way, so not a regression, just an inconsistency.
  6. Deleting the 1/X subtemplate id removes it from the template API; any package template referencing that id silently loses the entry. Worth a CHANGELOG line.

Pre-existing, surfaced while reviewing

  1. ScalarDistributionInputEditor.java:718 branches on template IDs ("BoundedReal", "BoundedInt", "OffsetReal", "OffsetInt") to decide what to offer, rather than on registered data.
  2. PriorInputEditor imports concrete Dirichlet and strips it from the list by ID string (:220).
  3. methods.csv still lists beast.base.inference.distribution.OneOnX and contains no spec-package distributions at all, so generated methods text is stale across the board — not just for the entry this PR removes.

@walterxie

Copy link
Copy Markdown
Member Author

I cannot reproduce any issues about Dirichlet in this PR:

XML looks fine and it is working.

Screenshot 2026-08-27 at 15 23 08

dirichlet.xml

@alexeid

alexeid commented Aug 27, 2026

Copy link
Copy Markdown
Member

Sorry, check my edit above.

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