Add pooling argument to analyse_mi_data() for unknown imputation methods - #52
Add pooling argument to analyse_mi_data() for unknown imputation methods#52bailliem wants to merge 15 commits into
Conversation
Tags the constructed stand-in method with class rbmiUtils_standin so print.analysis()/summary.analysis() report it as "<not supplied; pooling = "...">" instead of fabricating Bayesian/condmean provenance (F1). Adds coverage for the bootstrap pooling-only path and folds a rbmi::pool() success assertion into the jackknife test (F2). Guards make_standin_method() against bootstrap with fewer than 2 imputations, which previously built n_samples = 0 and pooled to NAs silently (F3), and adds a default switch() arm that aborts informatively on an unrecognized pooling string (F4). Also removes a stray double blank line, documents the condmean else-branch assumption in get_pooling(), and notes in analyse_mi_data()'s @param pooling that the returned method element is a constructed stand-in, not the true method (F5, F6).
compute_rubin_diagnostics() is @nord so no help topic exists; roxygen link syntax [compute_rubin_diagnostics()] emitted unresolved-link warnings on document(). Includes RoxygenNote bump to 7.3.3.
|
Thank you @bailliem Overall, this looks good. One design comment, though: I'm not convinced it's a good idea to have If backward compatibility is a must, I think it would be cleaner to keep a single method argument. It could accept either:
If backward compatibility is not a requirement, I would simply keep a single method argument that accepts the name of the pooling strategy as a character string. |
luwidmer
left a comment
luwidmer
left a comment
There was a problem hiding this comment.
I tend to agree with @wwojciech about the argument (unless we are planning to deprecate and then in a subsequent version remove the old one).
In addition, is there some way to prevent use of e.g., pooling="rubin" on data that was generated by pooling="bootstrap"? Seems to me like there's potential to introduce silent/subtle errors there?
3 participants
Closes #50
Summary
analyse_mi_data()can now analyse an already-imputed (ADMI) dataset when the imputation method is unknown, via a newpoolingargument — addressing the scenario raised in #50 where themethodobject is unavailable.poolingargument ("rubin","bootstrap","jackknife","bmlmi"): supply eithermethod(unchanged behaviour, fully backward compatible) orpoolingdirectly. Both together must agree; neither errors informatively.get_pooling(method)— the single source of truth mapping rbmi method objects to pooling strategies (as proposed in analyse_mi_data() when the exact imputation method is unknown. #50).poolingis supplied, the data defines the number of imputations (non_samplescheck) and an internal stand-in method object sized to the data is constructed sorbmi::pool()validation passes.print()/summary()label these objects<not supplied; pooling = "rubin">rather than claiming a method that was never given.pooling = "rubin"is the documented recommendation for data of unknown provenance;"bmlmi"alone errors (D not inferable from data);"bootstrap"with < 2 imputations errors rather than silently pooling to NAs.docs/superpowers/; also fixes pre-existing roxygen unresolved-link warnings for@noRdinternals inard_conversion.R.The
pooling/get_pooling()design is deliberately shaped to be liftable into rbmi core per openpharma/rbmi#589.Test plan
test-analyse_mi_data.R(34 new): validation error paths, method/pooling conflict, path-equivalence (pooling-only vsmethodproduce identical pooled results), end-to-endrbmi::pool()for rubin/bootstrap/jackknife stand-ins, provenance labelling, guards.test-get_pooling.Rcovering all four method classes and error paths.test-utils.Rfailures are a local rstan/Rcpp toolchain issue unrelated to this change.