Merge poissonconsulting/dev: refactors, bug fixes, dependency and test cleanup#478
Merged
Conversation
The small-sample-bias vignette depended on mle.tools, which has been removed from CRAN (bcgov#474), preventing it from knitting. It has been transferred to the ssdvignettes package. Removes the vignette source, rendered PDF and stub, the _pkgdown.yml article entry, the release question, and the now-unused mle.tools and reshape2 Suggests. Fixes bcgov#474 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove small sample bias vignette (moved to ssdvignettes)
…ot2 aes - mean_weighted_values() now reads data$weight rather than misusing the logical weight flag as the weight vector, so weighted fits get weighted starting values (uniform-weight results are unchanged). - Remove duplicate ssd_elgumbel() definition (the first also carried an incorrect ssd_einvpareto() @examples); sync man/ssd_e.Rd. - Replace deprecated ..density.. with after_stat(density) in StatSsdpoint and StatSsdsegment default aesthetics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- dplyr is used unconditionally throughout R/, so move it from Suggests to Imports (a Suggests-only dependency used without a guard is a latent failure when dplyr is absent). - Replace the sole plyr::summarise call in GeomXribbon with base data.frame and drop plyr from Imports, NAMESPACE, and the package import. - Remove unused Suggests: magrittr, reshape2, tidyselect (no references in R/, tests/, or vignettes/). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The six model-averaged multi functions each hard-coded the full set of
~33 distribution parameters: ssd_pmulti/ssd_qmulti/ssd_rmulti as a
signature plus a verbatim forwarding call, and pmulti_ssd/qmulti_ssd/
rmulti_ssd as a signature plus a verbatim list(). Adding or renaming a
distribution parameter meant editing six identical blocks, where a typo
could silently diverge.
- Make .relist_estimates() order-robust by reshaping the flat arguments
by name rather than position. The internal list() blocks were
previously load-bearing because relist() is positional; with the
name-based reshape the order parameters arrive in no longer matters.
- Collapse the internal pmulti_ssd/qmulti_ssd/rmulti_ssd to
function(q/p/n, ...) forwarding list(...) to .relist_estimates(), and
rename qmulti_ssd's first argument q -> p (it is a probability).
- Replace the forwarding bodies of the exported ssd_pmulti/ssd_qmulti/
ssd_rmulti with do.call(<dist>, c(list("multi"), as.list(environment()))).
Their explicit signatures are retained so argument names and defaults
remain validated and documented.
multi.R drops from 632 to 296 lines. Output verified byte-identical to
the previous implementation across p/q/r, mixtures, unnormalised
weights, seeded random generation, round-trips, the non-canonical
argument-order path, and the zero-weight error.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The model-averaged CDF was assembled by pasting R source into a string and eval(parse())-ing it, and tdist() resolved a function by parsing its name. Both defeat static analysis and are fragile to formatting. - pmulti_fun() now returns a closure that sums weight * pXXX_ssd(q, ...) over the component distributions (using get() for lexical lookup of the component CDF) rather than building and parsing a source string. The now-unused string helpers value_args() and pmulti_dist() are removed. - tdist() resolves ssd_p<dist> with match.fun() instead of eval(parse()). Output verified identical to the string-based implementation across the p/q/r paths, distribution mixtures, lower.tail/log.p, round-trips and seeded random generation; match.fun() resolves the same ssd_p<dist> function as eval(parse()) for every distribution. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Extract the repeated percent->proportion deprecation-and-validation block (duplicated across ssd_hc.list/.fitdists/.fitburrlioz and predict.fitdists/.fitburrlioz) into a single .hc_proportion() helper. - ssd_plot(): validate that hc is numeric (before chk_subset) and that decimal.mark is a string (previously only big.mark and suffix were checked, while ssd_plot_data() checked decimal.mark but not suffix). - ssd_plot_data(): validate that suffix is a string. - .pd()/.qd(): replace the bare stop() length-1 assertion with an err() carrying a message. - Fix incorrect function names in two ssd_hp() deprecation messages (they referred to ssd_hc() and ssd_hp_bcanz()). - Fix "can not" -> "cannot" in the GeomXribbon aesthetics error. The .hc_proportion() helper is verbatim the previous inline logic; output verified identical across the supplied / not-supplied / invalid percent and proportion paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
None of these change behaviour; they remove unused arguments and dead or fragile code flagged in the code review. - no_hcp(): drop the unused `hc` parameter (both call sites pass none). - bburrIII3()/blnorm_lnorm()/bllogis_llogis(): drop the unused leading `x` parameter. These are dispatched by bdist() via named arguments (data, min_pmix, range_shape1, range_shape2), so `x` was never filled. - sample_parametric(): drop the self-referential `args = args`, `weighted = weighted`, `censoring = censoring` default bindings; the sole caller (generate_data) always supplies them. - clean_hcp(): guard the min_pboot mask with !is.na(pboot), matching replace_min_pboot_na(); identical when pboot has no NA, NA-safe when it does (previously any(NA < min_pboot) could error the if()). - hcp_combine_samples(): drop the redundant nboot1 alias (nboot is a function argument, not a column, so it resolves directly like the sibling weight/geometric arguments in the same call). - hcp_noci(): remove a trailing comma in the tibble() call. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove repeated 33-parameter blocks in multi.R
…messages Deduplicate proportion handling, tighten validation and messages
…al-parse Replace eval(parse()) metaprogramming with direct dispatch
The previous commit replaced the nboot1 alias with a bare nboot inside dplyr::summarise(), but the bound hcp tibble has an nboot column, so the bare reference resolved to that column vector rather than the scalar function argument, breaking combine_samples()/rep(). Reference the argument explicitly via .env$nboot (the original nboot1 alias existed to dodge exactly this data-mask shadowing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The mean_weighted_values() fix means weighted fits now derive starting values from properly weighted data, which shifts the converged estimates at the 6th significant figure (e.g. 0.546531 -> 0.546532 in hcallw10, meanlog 0.6239 -> 0.623898 in the print snapshot). Regenerated the two affected snapshots; no other snapshots changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
update range_shape1 and range_shape2 parameter descriptions
…-fixes Fix weighted starting values, duplicate definition, and dependency cleanup
…leanup Remove dead parameters and tidy minor cleanups
ci: Fix codecov
abind is in Imports with an @importFrom (and NAMESPACE entry) but abind() is never called anywhere in the package. Remove it. dplyr (Imports) and the removal of reshape2 are already present on main, so this completes the remaining dependency-declaration cleanup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove unused abind dependency
The ssd_hc arithmetic_samples test ran its bootstrap with no set.seed, so its ci (se, lcl, ucl) depended on the ambient global RNG state left by preceding tests. Refactoring that shifted upstream RNG consumption broke the snapshot while est (the deterministic MLE) stayed fixed. Wrap the ssd_hc call in withr::with_seed(102, ...) to match the sibling ci tests, and regenerate the snapshot under that seed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The censor snapshots were regenerated for ssddata's new Medium column, but CI installed a CRAN ssddata without it, so ssd_censor_data output lacked the column and the snapshots failed. Add a Remotes entry so CI installs the dev ssddata that provides Medium. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tic-samples-seed Seed arithmetic_samples ci test for reproducibility
Closes #173 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add CODEOWNERS naming @joethorley
Collaborator
Author
Summary of changesThis PR merges Bug fixes
Refactoring / dead-code removal
Dependencies
Tests / CI
Docs
Reviewer notes (fork-specific, may not belong upstream)
|
aazizish
approved these changes
Jul 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings the accumulated development work on
poissonconsulting:devintobcgov/ssdtools. A detailed summary of the changes follows in a comment below.🤖 Generated with Claude Code