Repository navigation
Conversation
Reuse classical summaries, plots and controls; add model comparisons, weight functions, profiles, exports and reproducible R code. Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Use the BayesTools prior printer and guard optional upgrade operations.
Replace legacy selection-model tests with bcg coverage; refresh saved analyses and plot references.
|
build builder bot |
✅ Build Complete!Your JASP module bundles have been successfully built for macOS (ARM) and Windows. 👉 Click here to download the artifacts (Scroll to the bottom of the page to find the "Artifacts" section). |
|
I understand the backwards compatibility issue, but I assume there should be a better way of letting users know their old files are not working. I got this when loading the JASP file from the data library |
|
it seems this was an issue with some old module version lingering around. So there is no error anymore. :) |
|
shouldn't "P-value" in the qml be lowercase p? |
juliuspfadt
left a comment
juliuspfadt
left a comment
There was a problem hiding this comment.
Review of the selection-models rewrite. The PR build (0.98.0) was run through jaspTools on a simulated 80-study dataset with publication bias, plus about 350 edge-case configurations (bad SE / p-values / sample sizes, tiny subgroups, junk in cutoff and parameter fields, all function types, optimizer settings, model-list edge cases). No hard crashes; most failures are graceful. Inline comments cover the correctness items; things I could not anchor inline:
Not anchored to the diff
DESCRIPTION:metaforhas no minimum version, butselmodel(subset=)and two-value betastepsarrived in metafor 4.8-0 (metafor NEWS). Suggestmetafor (>= 4.8-0).weightris removed from DESCRIPTION but is still inrenv.lock.- Raw R error in the forest plot (shared code,
.maPrintPValueinclassicalmetaanalysis-helpers.R:159):if (pValue < 0.001)has no NA guard, so the forest plot printsError in if (pValue < 0.001) { : missing value where TRUE/FALSE needed(rest of output fine). Repro with Forest plot > Model information on: Custom, one Truncation row with cutoff10; or Custom Estimated truncation with parameters1, NA; or Beta / Negative exponential power with several settings; or Advanced > Convergence relative tolerance0.01. - Performance: with 80 studies, Half-normal takes about 25 s, Negative exponential about 90 s, Beta 2-4 min (two-sided about double), and ten mixed models over an hour, because each candidate needs several
selmodelfits (plainmetafor::selmodeltakes 5-19 s for Half-normal and Beta alone). - Unit tests: analyses 4 (Beta, delta2 at its 1e-5 bound) and 6 (logistic with precision) differ at about 1e-4 relative between package versions in CI. Consider a looser tolerance or dropping the Beta candidate from the snapshot.
- The failing
RoBMAtests in the CI "latest packages" job (r2z,set_default_priorsnot exported) are unrelated to this PR.
| biasTest <- createJaspTable(title = gettext("Test of Publication Bias")) | ||
| biasTest$position <- 2 | ||
| fitTests[["biasTest"]] <- biasTest | ||
| .smRowPublicationBiasTest <- function(fit) { |
There was a problem hiding this comment.
Publication-bias test is shown even when it is not valid. .smRowPublicationBiasTest always emits fit$LRT/fit$LRTp, while .smInferenceMessages adds the footnote "not applicable with fixed selection parameters" when !.smNoBiasNested(fit) || fit$LRTdf == 0.
Repro: Custom, Step (.025, .50), selection parameters 1, NA, 0.1 -> the tests table shows Publication bias LR(1) = 8.58, p = .0034 next to that note. With all weights fixed (1, .5, .2) it shows LR = 0.00 and a blank p. The same statistic reaches the forest plot through .smPrintBiasTest (selectionmodels-plots.R:46).
Suggest returning NULL (and no forest label) under that condition.
There was a problem hiding this comment.
Fixed: inapplicable tests are now omitted from both the table and forest plot.
| return(NULL) | ||
|
|
||
| labels <- if (stepFunction) { | ||
| paste0("[", head(c(0, fit$steps), -1), ", ", fit$steps, ")") |
There was a problem hiding this comment.
Interval labels are left-closed [a, b), but selmodel assigns p-values to right-closed intervals (a, b], and the frequency table below uses a < p <= b. For 4PSM the estimates table says [0, 0.025) while the frequency table says 0 < p <= 0.025, so a study with p = .025 appears to belong to different intervals. Suggest (a, b].
There was a problem hiding this comment.
Fixed: labels now use (a, b], matching metafor's interval assignment.
| return(fit) | ||
|
|
||
| # Equal- and fixed-effects models have no tau2 to profile. | ||
| needed <- options[["confidenceIntervals"]] && .smHasEstimatedHeterogeneity(fit) && |
There was a problem hiding this comment.
needed depends on the table-level options[["confidenceIntervals"]], so unticking Statistics > Confidence intervals also removes the τ/τ² interval from the forest plot labels (𝜏 = 0.51 instead of 𝜏 = 0.51 [0.31, 1.02]), unlike the classical forest plot. needed should also be TRUE when the forest heterogeneity estimates are requested.
There was a problem hiding this comment.
Fixed: forest intervals are computed independently of the table's confidence-interval checkbox.
| # GLMM always uses ML; drop the REML column | ||
| if (inherits(fit, "rma.glmm") && "REML" %in% colnames(fitStats)) | ||
| # GLMM and selection models only supply ML fit statistics. | ||
| if (inherits(fit, c("rma.glmm", "rma.uni.selmodel")) && "REML" %in% colnames(fitStats)) |
There was a problem hiding this comment.
The REML column is only dropped for rma.glmm / rma.uni.selmodel. The unadjusted "None" candidate is a plain rma.uni, so its Fit Measures table lists both ML and REML rows, and its dev is relative to the saturated model (e.g. 124.6 on a simulated 80-study dataset) while Model Comparison reports -2*logLik (-27.5) for the same model. The method is ML, so the REML row is meaningless and the deviance is inconsistent between the two tables.
There was a problem hiding this comment.
Fixed: the None candidate now reports ML only, with deviance consistently defined as -2 log-likelihood.
| if (!.smHasEstimatedHeterogeneity(fit)) | ||
| return(NULL) | ||
|
|
||
| row <- data.frame( |
There was a problem hiding this comment.
When LRT.tau2 is not finite this still emits a "Residual heterogeneity" row with a blank statistic and p-value and no footnote (seen with Half-normal + inverse sample size precision on meta-regression data). It looks like a missing result. Return NULL or add a note.
There was a problem hiding this comment.
Fixed: unavailable LRT rows are omitted, with an explanatory table footnote.
| if (!identical(cache[["key"]], key)) | ||
| cache <- list(key = key, scopes = list()) | ||
|
|
||
| scopes <- list(full = seq_len(nrow(dataset))) |
There was a problem hiding this comment.
The full scope (reference + every candidate fit + profile CIs / exports) is always fitted, even when a subgroup is chosen and "Include full dataset" is off; it is then filtered out by .smVisibleScopes. With several candidates this multiplies runtime on every cache invalidation. Skip it when it is not visible.
There was a problem hiding this comment.
Fixed: the full dataset is fitted only when requested or when no subgroup is selected.
| if (any(!is.finite(fit$se))) | ||
| gettext("Coefficient standard errors could not be estimated. Wald tests and coefficient confidence intervals are unavailable; review the fitting warnings before interpreting this model."), | ||
| if (options[["noSelection"]] != "") | ||
| gettextf( |
There was a problem hiding this comment.
Counts are interpolated without ngettext() ("Selection applies to 1 studies; ...", "Affected selected studies: 1"). The translation guidelines require plural forms.
There was a problem hiding this comment.
Fixed: study-count messages now use ngettext().
| .smFrequencyTable <- function(jaspResults, options) { | ||
|
|
||
| if (!.smCheckReady(options)) | ||
| if (!options[["weightFunctionPValueFrequencyTable"]]) |
There was a problem hiding this comment.
Before variables are assigned the frequency builder shows the text "P-value frequencies require a fitted selection model with p-value cutoffs." instead of an empty table skeleton like the other builders. That is misleading when the cause is just missing input.
There was a problem hiding this comment.
Fixed: an empty table skeleton is now shown.
| rows <- scopes[[scope]] | ||
| scopeData <- droplevels(dataset[rows, , drop = FALSE]) | ||
| label <- if (scope == "full") { | ||
| gettext("Full Dataset") |
There was a problem hiding this comment.
"Full Dataset" here vs "Full dataset" in classicalmetaanalysis-fit.R:56: same concept, two translation strings and inconsistent export column names.
There was a problem hiding this comment.
Fixed: selection models now use the shared 'Full dataset' string.
| # print notes & fill empty spots | ||
| priors <- lapply(priors, function(x) c( | ||
| if (length(x) > 0) sapply(x, print, short_name = options[["shortenPriorName"]], silent = TRUE), | ||
| if (length(x) > 0) sapply(x, BayesTools:::print.prior, short_name = options[["shortenPriorName"]], silent = TRUE), |
There was a problem hiding this comment.
Unrelated to selection models: this switches to the unexported BayesTools:::print.prior. That couples the module to a BayesTools internal; print() dispatches to the same method via the public generic. Consider reverting or splitting this out of the PR.
There was a problem hiding this comment.
Retained intentionally: metaBMA also registers print.prior, and generic dispatch can select its incompatible method.
There was a problem hiding this comment.
Replaced the ::: calls with public S3-method lookup scoped to BayesTools. Regression coverage verifies correct printing when another package registers a conflicting print.prior method.
|
Non-helpful error message: |
|
The model naming in the qml is not updating, leading to a mismatch between the model names in the output and the model names in the qml. |
|
Error with convergence tolerance and forest plot |
|
Forest plot error with truncation and cutoff 10 |
|
build builder bot |
✅ Build Complete!Your JASP module bundles have been successfully built for macOS (ARM) and Windows. 👉 Click here to download the artifacts (Scroll to the bottom of the page to find the "Artifacts" section). |
|
okay, the forest plot errors are fixed. |
juliuspfadt
left a comment
juliuspfadt
left a comment
There was a problem hiding this comment.
Second pass on 04177a7. Thanks for the fixes in f4eac23: the earlier threads on the bias-test row, interval labels, forest tau CI, REML row, ngettext, the frequency-table empty state, the metafor version and the NA parsing look addressed, and the forest plot errors were confirmed fixed on the new build.
Still open from the first review: the upgrade-message version pair (0.95.5 vs 0.97.5), stale twoSided on truncation rows (R/selectionmodels-options.R), BayesTools:::print.prior in the RoBMA tables, and weightr entries still in renv.lock.
New inline comments are below. One cannot be anchored to a line:
.claude/Selection Models - bcg (sample-size workaround).jasp(1 MB binary) is committed inside the instruction directory. It is not a test fixture (those live intests/testthat/jaspfiles) and the repo rules say not to commit large data or temporary files. Please remove it from the PR.
| error = function(e) .smError(conditionMessage(e)) | ||
| ) | ||
|
|
||
| attr(value, "selectionWarnings") <- unique(warnings) |
There was a problem hiding this comment.
.smCapture sets the selectionWarnings attribute outside its tryCatch, so a NULL result fails with attempt to set an attribute on NULL and aborts the whole analysis (stack trace) instead of being skipped. .smCapture(.maExportPredictedDataFrame(fit)) in selectionmodels-exports.R can receive NULL: .maExportListRmaDataFrame returns NULL when there are no prediction columns. .smCapture(NULL) errors on its own, so the failure is easy to confirm. Suggest guarding the attr<- call with if (!is.null(value)).
There was a problem hiding this comment.
Fixed: .smCapture() now guards NULL results before assigning attributes. Added regression coverage for NULL returns, warnings, and errors.
| args$decreasing <- spec[["decreasing"]] | ||
|
|
||
| if (options[["pValue"]] != "") | ||
| args$pval <- dataset[[options[["pValue"]]]] |
There was a problem hiding this comment.
Supplied p-values are passed through selmodel's pval argument. metafor's NEWS (4.6-0) describes it as "undocumented" and "highly experimental". If a later metafor release renames or removes it, analyses with a P-Value variable would fail, or the p-values would be ignored while the footnote still says they were supplied. Worth pinning with a unit test that supplied and computed p-values give identical fits, so a metafor change is caught by CI.
There was a problem hiding this comment.
Added CI regressions comparing supplied and computed p-values for both sidedness settings and effect directions. They also verify that changing supplied p-values changes the likelihood, catching silently ignored inputs.
| # get the p-value steps and p-values(so we don't have to search for them in the models) | ||
| steps <- .smGetCutoffs(options) | ||
| pVal <- .maGetInputPVal(dataset, options) | ||
| if (!is.null(fits) && !length(frequencyFits)) { |
There was a problem hiding this comment.
The frequency table is replaced by "P-value frequencies require a fitted selection model with p-value cutoffs." whenever the fit fails. That includes an empty p-value interval or an optimizer error, which is exactly when a user needs the counts to see what is wrong. Repro: Custom, Step (.025, .50), selection parameters 1, 0, 0 (optimizer error), or a 4PSM on data with no studies in an interval. The counts do not need the fit, and the p-value table is already computed in .smFitOne and then discarded. Suggest building the table from the cutoffs and p-values independently of fit success (as the legacy .smFrequencyTable did), and showing the message only when no valid cutoffs exist.
There was a problem hiding this comment.
Fixed: cutoff frequencies are retained independently of fitting success. Verified that counts remain displayed after empty-interval and optimizer errors.
| if (length(sampleSizeVariables)) { | ||
|
|
||
| } else if (!jaspBase::isTryError(fit)) { | ||
| keep <- stats::complete.cases(dataset[, sampleSizeVariables, drop = FALSE]) |
There was a problem hiding this comment.
dataset[, sampleSizeVariables] (and dataset[[spec$sampleSize]] in .smFitCandidates) assume the nested sample-size DropDown variable is present in dataset. If it is not delivered (variable renamed or deleted, or the nested control not marked for encoding), this fails with the raw undefined columns selected. A validation message such as "Sample-size variable not found in the data" would be expected instead. The "sample-size workaround" file in this PR suggests it has already bitten once.
There was a problem hiding this comment.
Fixed: missing sample-size columns now produce an actionable message asking the user to assign an available variable, before indexing the dataset.
| functionName: "SelectionModels" | ||
| fromVersion: "0.97.5" | ||
| toVersion: "0.98.0" | ||
| msg: qsTr("Selection Models was reworked in Meta-Analysis module 0.98.0. Refreshing this analysis replaces the saved results and may reset incompatible settings. To rerun the original analysis, use JASP 0.98.1 with Meta-Analysis module 0.95.5.") |
There was a problem hiding this comment.
Related to the startup-message point above: the upgrade drops modelAutomaticallyJoinPValueIntervals (default TRUE in legacy analyses) and the correlation/N input (measures, sampleSize, transformCorrelationsTo), and the new engine has no equivalent for either. A legacy analysis with default cutoffs and few significant studies relied on interval joining; after refresh it can fail with "... contains no selected studies ... intervals are not merged automatically", and a correlation + N analysis becomes "not ready". The message only says settings "may reset". It would help to say explicitly what is dropped.
There was a problem hiding this comment.
This thread is still open, so a question rather than a request: why are these options dropped without an equivalent? If it is intentional (the metafor engine has no interval joining and no correlation/N input), could the upgrade msg say so explicitly, e.g. that interval joining and correlation + N input are no longer supported? As it stands the message only says settings "may reset", and a legacy analysis with default cutoffs can end in "intervals are not merged automatically" with no hint why.
There was a problem hiding this comment.
These changes are intentional parts of the full rework: the new interface accepts effect sizes with standard errors and leaves interval selection explicit. I’m retaining the general upgrade warning, which covers incompatible settings and points users to the previous version for reproducing their original analysis.
|
|
||
| CheckBox | ||
| { | ||
| name: "weightFunctionEstimates" |
There was a problem hiding this comment.
weightFunctionEstimates (checked by default), weightFunctionPlot and weightFunctionPValueFrequencyTable are defined in this shared component inside a group that is only visible for selection models. Hidden controls still create their options, so every other analysis using the component (Meta-Analysis, Multilevel, GLMM, Mantel-Haenszel-Peto) now carries three dead options, one of them defaulting to TRUE, in saved files and generated help. Consider moving them to SelectionModels.qml or a selection-only component.
There was a problem hiding this comment.
Fixed: moved all three weight-function controls into SelectionModels.qml, so other analyses no longer create these options.
There was a problem hiding this comment.
These controls remain in saved option lists, as with other shared components. I checked ANOVA’s shared controls, which use the same analysis-dependent visibility pattern. They are hidden outside Selection Models, and only its backend reads them. I’m keeping them in the shared Statistics component.
|
@juliuspfadt, replies to the unanchored review points:
The additional lockfile and tolerance updates are prepared locally. |
|
Regarding the startup warning: the module already defines an upgrade warning. @boutinb, could you check why Desktop doesn't display it when loading the legacy file and address the warning delivery? |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #479 +/- ##
===========================================
+ Coverage 40.24% 53.59% +13.34%
===========================================
Files 24 82 +58
Lines 13331 18001 +4670
===========================================
+ Hits 5365 9647 +4282
- Misses 7966 8354 +388 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Codex <noreply@openai.com>
|
Removed |
|
The standalone |
Co-Authored-By: Codex <noreply@openai.com>
|
build builder bot |
|
this does not seem fixed: |
|
The model naming is also still not fixed: #479 (comment) |
|
On the test tolerance: Why exact comparison plus pinned x86-64 runners, reference BLAS and a custom workflow, instead of keeping the tolerance and the shared |
Co-Authored-By: Codex <noreply@openai.com>
|
Regarding the error message: Fixed in 4949634. Weights |
|
Regarding model naming: Fixed in 4949634. Reproduced after deleting a middle row. The QML fix refreshes display numbers when rows load, are added, or are removed, while preserving internal model IDs. Verified numbering and "Show models" filtering with headless Qt and R output checks. |
|
Regarding tolerance and CI configuration: Agreed that these boundary estimates need numerical tolerance. Restored Local verification passed: 327 suite checks, 15 expected skips, 59 focused regression checks, and QML checks. Reference snapshots remain unchanged. |
3 participants
Full rewrite of selection models (now rely on metafor::selmoded) and breaking backwards compatibility