search

ropensci / software-review

rOpenSci Software Peer Review.
286 stars 104 forks source link

dynamite: Bayesian Modeling and Causal Inference for Multivariate Longitudinal Data #554

Closed santikka closed 1 year ago

santikka commented 1 year ago

Date accepted: 2022-12-13 Submitting Author Name: Santtu Tikka Submitting Author Github Handle: !--author1-->@santikka<!--end-author1-- Other Package Authors Github handles: !--author-others-->@helske<!--end-author-others-- Repository: https://github.com/santikka/dynamite Version submitted: 0.0.1 Submission type: Stats Badge grade: gold Editor: !--editor-->@noamross<!--end-editor-- Reviewers: @nicholasjclark, @lucymcgowan

Due date for @nicholasjclark: 2022-09-27 Due date for @lucymcgowan: 2022-10-20

Archive: TBD Version accepted: TBD Language: en

Package: dynamite
Title: Bayesian Modeling and Causal Inference for Multivariate
    Longitudinal Data
Version: 0.0.1
Authors@R: c(
    person("Santtu", "Tikka", , "santtuth@gmail.com", role = c("aut", "cre"),
           comment = c(ORCID = "0000-0003-4039-4342")),
    person("Jouni", "Helske", , "jouni.helske@iki.fi", role = "aut",
           comment = c(ORCID = "0000-0001-7130-793X"))
  )
Description: Easy-to-use and efficient interface for 
  Bayesian inference of complex panel (time series) data. The package supports 
  joint modeling of multiple measurements per individual, time-varying and
  time-invariant effects, and a wide range of discrete and 
  continuous distributions. Estimation of the models is carried out via 'Stan'.
License: GPL (>= 3)
URL: https://github.com/santikka/dynamite
BugReports: https://github.com/santikka/dynamite/issues
Depends: 
    R (>= 4.1.0)
Imports: 
    bayesplot,
    checkmate,
    cli,
    data.table (>= 1.14.3),
    dplyr,
    glue,
    ggplot2,
    MASS,
    posterior,
    rlang,
    rstan (>= 2.26.11),
    stats,
    tidyr,
    utils
Suggests: 
    covr,
    knitr,
    plm,
    rmarkdown,
    testthat (>= 3.0.0)
VignetteBuilder: 
    knitr
Config/testthat/edition: 3
Encoding: UTF-8
Roxygen: list(markdown = TRUE, roclets = c ("namespace", "rd",
    "srr::srr_stats_roclet"))
RoxygenNote: 7.2.1
LazyData: true
LazyDataCompression: xz

Pre-submission Inquiry

General Information

The package is mainly intended for applied researchers working with complex panel data. Panel data is common in many scientific fields, especially in sociology and econometrics. For example, analysing individual-level life-course data is valuable for assessing the effects of policy reforms and other interventions.

The dynamite R package provides easy-to-use interface for Bayesian inference of complex panel (time series) data comprising of multiple measurements per multiple individuals measured in time. The main features distinguishing the package and the underlying methodology from many other approaches are:

There are several R packages in CRAN focusing on panel data analysis including but not limited to:

However, to the best of our knowledge, there are no other R packages (or software in general) that support all features of dynamite simultaneously. Thus The first implementation of a novel algorithm seems most applicable.

Not applicable.

Badging

Gold.

We designed dynamite from the ground up with the standards in mind and thus we think the package fulfills all four aspects. We have been able to comply with 129 standards with only 42 N/A standards across the "Bayesian and Monte Carlo" and "Regression and Supervised Learning" categories. The package is very general and capable of supporting a wide range of data and model structures as demonstrated by the examples and the package tests. In our opinion, the internal structure of the package is also well motivated and compartmentalized. We have also tried to carefully select the package dependencies and keep them to a minimum.

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

Code of conduct

ropensci-review-bot commented 1 year ago

Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type @ropensci-review-bot help for help.

ropensci-review-bot commented 1 year ago

:rocket:

Editor check started

:wave:

ropensci-review-bot commented 1 year ago

Oops, something went wrong with our automatic package checks. Our developers have been notified and package checks will appear here as soon as we've resolved the issue. Sorry for any inconvenience.

mpadge commented 1 year ago

@ropensci-review-bot check package

ropensci-review-bot commented 1 year ago

Thanks, about to send the query.

ropensci-review-bot commented 1 year ago

:rocket:

Editor check started

:wave:

ropensci-review-bot commented 1 year ago

Checks for dynamite (v0.0.1)

git hash: a8d932ca

Important: All failing checks above must be addressed prior to proceeding

Package License: GPL (>= 3)

1. rOpenSci Statistical Standards (srr package)

This package is in the following category:

:heavy_check_mark: All applicable standards [v0.1.0] have been documented in this package (129 complied with; 42 N/A standards)

Click to see the report of author-reported standards compliance of the package with links to associated lines of code, which can be re-generated locally by running the srr_report() function from within a local clone of the repository.

2. Package Dependencies

Details of Package Dependency Usage (click to open)

The table below tallies all function calls to all packages ('ncalls'), both internal (r-base + recommended, along with the package itself), and external (imported and suggested packages). 'NA' values indicate packages to which no identified calls to R functions could be found. Note that these results are generated by an automated code-tagging system which may not be entirely accurate. |type |package | ncalls| |:----------|:----------|------:| |internal |base | 1102| |internal |dynamite | 418| |internal |graphics | 21| |internal |methods | 3| |imports |utils | 81| |imports |stats | 59| |imports |dplyr | 29| |imports |rlang | 16| |imports |checkmate | 11| |imports |glue | 11| |imports |cli | 6| |imports |ggplot2 | 4| |imports |posterior | 4| |imports |tidyr | 4| |imports |rstan | 3| |imports |data.table | 2| |imports |bayesplot | 1| |imports |MASS | NA| |suggests |covr | NA| |suggests |knitr | NA| |suggests |plm | NA| |suggests |rmarkdown | NA| |suggests |testthat | NA| |linking_to |NA | NA| Click below for tallies of functions used in each package. Locations of each call within this package may be generated locally by running 's <- pkgstats::pkgstats()', and examining the 'external_calls' table.

base

c (108), list (96), length (69), paste0 (66), args (53), for (47), attr (44), data.frame (39), as.list (32), match.call (32), rep (31), seq_len (25), unique (25), do.call (22), character (19), which (18), vapply (17), is.na (16), names (16), logical (15), seq_along (15), drop (13), by (12), deparse1 (11), is.null (10), mean (10), nzchar (10), all (9), apply (9), debug (9), lapply (9), parent.frame (9), as.integer (7), integer (7), rank (7), seq.int (7), sort (6), unlist (6), array (5), assign (5), colnames (5), dim (5), log (5), message (5), mode (5), nrow (5), try (5), vector (5), as.numeric (4), call (4), diff (4), eval (4), gsub (4), as.data.frame (3), cbind (3), I (3), identical (3), setdiff (3), sub (3), sum (3), any (2), aperm (2), expand.grid (2), levels (2), max (2), ncol (2), seq (2), structure (2), suppressWarnings (2), t (2), beta (1), class (1), det (1), diag (1), duplicated (1), get (1), gl (1), gregexec (1), ifelse (1), intersect (1), is.factor (1), is.finite (1), match (1), min (1), new.env (1), numeric (1), parse (1), paste (1), prod (1), range (1), regmatches (1), replace (1), replicate (1), row.names (1), sample (1), sample.int (1), signif (1), strsplit (1), substitute (1), typeof (1), union (1), warning (1), which.max (1), which.min (1), with (1)

dynamite

ifelse_ (84), paste_rows (41), get_responses (17), data_lines_default (10), get_predictors (10), onlyif (10), model_lines_default (9), warning_ (9), prepare_channel_default (8), formula_rhs (6), get_quoted (5), as.data.frame.dynamitefit (4), get_families (4), has_past (4), coef.dynamitefit (3), evaluate_specials (3), get_formulas (3), assign_deterministic (2), complete_lags (2), create_blocks (2), cs (2), default_priors (2), default_priors_categorical (2), deterministic_response (2), extract_lags (2), extract_nonlags (2), find_lags (2), formula_lhs (2), formula_past (2), formula_terms (2), full_model.matrix (2), full_model.matrix_predict (2), get_originals (2), get_terms (2), indenter_ (2), join_dynamiteformulas (2), lag_ (2), parse_global_lags (2), parse_lags (2), parse_new_lags (2), parse_singleton_lags (2), prepare_eval_envs (2), prepare_lagged_response (2), stop_ (2), which_deterministic (2), which_stochastic (2), abort_factor (1), abort_negative (1), abort_nonunit (1), add_dynamiteformula (1), as_data_frame_alpha (1), as_data_frame_beta (1), as_data_frame_corr_nu (1), as_data_frame_default (1), as_data_frame_delta (1), as_data_frame_lambda (1), as_data_frame_nu (1), as_data_frame_omega (1), as_data_frame_omega_alpha (1), as_data_frame_phi (1), as_data_frame_sigma (1), as_data_frame_sigma_nu (1), as_data_frame_tau (1), as_data_frame_tau_alpha (1), as_draws_df.dynamitefit (1), as_draws.dynamitefit (1), assign_initial_values (1), assign_lags (1), assign_lags_init (1), aux (1), check_ndraws (1), check_newdata (1), check_priors (1), clear_nonfixed (1), confint.dynamitefit (1), create_blocks.default (1), create_data (1), create_functions (1), create_generated_quantities (1), create_model (1), create_parameters (1), create_transformed_data (1), create_transformed_parameters (1), data_lines_bernoulli (1), data_lines_beta (1), data_lines_binomial (1), data_lines_categorical (1), data_lines_exponential (1), data_lines_gamma (1), data_lines_gaussian (1), data_lines_negbin (1), data_lines_poisson (1), drop_terms (1), drop_unused (1), dynamite (1), dynamitechannel (1), dynamitefamily (1), dynamiteformula (1), dynamiteformula_ (1), evaluate_deterministic (1), fill_time (1), fill_time_predict (1), fitted.dynamitefit (1), formula_specials (1), formula.dynamitefit (1), generate_random_intercept (1), generate_sim_call (1), get_code (1), get_code.dynamitefit (1), get_code.dynamiteformula (1), get_data (1), get_data.dynamitefit (1), get_data.dynamiteformula (1), get_priors (1), get_priors.dynamitefit (1), get_priors.dynamiteformula (1), get_special_term_indices (1), impute_newdata (1), increment_formula (1), initialize_deterministic (1), is_supported (1), is.dynamitefamily (1), is.dynamitefit (1), is.dynamiteformula (1), lags (1), lines_wrap (1), locf (1), mcmc_diagnostics (1), mcmc_diagnostics.dynamitefit (1), message_ (1), model_lines_bernoulli (1), model_lines_beta (1), model_lines_binomial (1), model_lines_categorical (1), model_lines_exponential (1), model_lines_gamma (1), model_lines_gaussian (1), model_lines_negbin (1), model_lines_poisson (1), ndraws.dynamitefit (1), nobs.dynamitefit (1), parameters_lines_bernoulli (1), parameters_lines_beta (1), parameters_lines_binomial (1), parameters_lines_categorical (1), parameters_lines_default (1), parameters_lines_exponential (1), parameters_lines_gamma (1), parameters_lines_gaussian (1), parameters_lines_negbin (1), parameters_lines_poisson (1), parse_data (1), parse_newdata (1), parse_past (1), parse_present_lags (1), plot_betas (1), plot_deltas (1), plot_nus (1), plot.dynamitefit (1), predict_dynamitefit (1), predict.dynamitefit (1), prepare_channel_bernoulli (1), prepare_channel_beta (1), prepare_channel_binomial (1), prepare_channel_categorical (1), prepare_channel_exponential (1), prepare_channel_gamma (1), prepare_channel_gaussian (1), prepare_channel_negbin (1), prepare_channel_poisson (1), prepare_common_priors (1), prepare_prior (1), prepare_splines (1), prepare_stan_input (1), values (1), verify_lag (1)

utils

data (79), capture.output (1), combn (1)

stats

formula (21), var (7), df (5), sd (4), D (3), model.matrix.lm (3), na.action (3), na.pass (3), offset (3), complete.cases (2), setNames (2), terms (2), sigma (1)

dplyr

bind_rows (12), filter (7), mutate (3), summarise (3), left_join (2), matches (1), n (1)

graphics

mtext (10), title (9), pairs (2)

rlang

caller_env (16)

checkmate

test_character (3), test_flag (3), test_string (3), test_int (2)

glue

glue (11)

cli

cli_abort (2), qty (2), cli_inform (1), cli_warn (1)

ggplot2

labs (3), position_dodge (1)

posterior

summarise_draws (2), as_draws (1), ndraws (1)

tidyr

expand_grid (2), full_seq (1), unnest (1)

methods

is (2), new (1)

rstan

extract (2), check_hmc_diagnostics (1)

data.table

setDT (1), setkeyv (1)

bayesplot

mcmc_combo (1)

**NOTE:** Some imported packages appear to have no associated function calls; please ensure with author that these 'Imports' are listed appropriately.

3. Statistical Properties

This package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing.

Details of statistical properties (click to open)

The package has: - code in R (100% in 30 files) and - 2 authors - 1 vignette - 6 internal data files - 14 imported packages - 36 exported functions (median 7 lines of code) - 404 non-exported functions in R (median 9 lines of code) --- Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages The following terminology is used: - `loc` = "Lines of Code" - `fn` = "function" - `exp`/`not_exp` = exported / not exported All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by [the `checks_to_markdown()` function](https://docs.ropensci.org/pkgcheck/reference/checks_to_markdown.html) The final measure (`fn_call_network_size`) is the total number of calls between functions (in R), or more abstract relationships between code objects in other languages. Values are flagged as "noteworthy" when they lie in the upper or lower 5th percentile. |measure | value| percentile|noteworthy | |:------------------------|-------:|----------:|:----------| |files_R | 30| 89.3| | |files_vignettes | 2| 85.7| | |files_tests | 11| 91.7| | |loc_R | 5956| 96.5|TRUE | |loc_vignettes | 877| 89.0| | |loc_tests | 2413| 95.3|TRUE | |num_vignettes | 1| 64.8| | |data_size_total | 2661885| 98.5|TRUE | |data_size_median | 349085| 96.0|TRUE | |n_fns_r | 440| 96.6|TRUE | |n_fns_r_exported | 36| 82.0| | |n_fns_r_not_exported | 404| 97.8|TRUE | |n_fns_per_file_r | 8| 83.4| | |num_params_per_fn | 2| 11.9| | |loc_per_fn_r | 9| 24.3| | |loc_per_fn_r_exp | 7| 13.5| | |loc_per_fn_r_not_exp | 9| 27.1| | |rel_whitespace_R | 4| 76.1| | |rel_whitespace_vignettes | 13| 68.1| | |rel_whitespace_tests | 9| 86.4| | |doclines_per_fn_exp | 37| 45.3| | |doclines_per_fn_not_exp | 0| 0.0|TRUE | |fn_call_network_size | 696| 96.9|TRUE | ---

3a. Network visualisation

Click to see the interactive network visualisation of calls between objects in package

4. goodpractice and other checks

Details of goodpractice checks (click to open)

#### 3a. Continuous Integration Badges [![R-CMD-check](https://github.com/santikka/dynamite/workflows/R-CMD-check/badge.svg)](https://github.com/santikka/dynamite/actions) **GitHub Workflow Results** | id|name |conclusion |sha | run_number|date | |----------:|:-------------|:----------|:------|----------:|:----------| | 2903919221|R-CMD-check |success |a8d932 | 295|2022-08-22 | | 2903919220|test-coverage |success |a8d932 | 295|2022-08-22 | --- #### 3b. `goodpractice` results #### `R CMD check` with [rcmdcheck](https://r-lib.github.io/rcmdcheck/) R CMD check generated the following note: 1. checking installed package size ... NOTE installed size is 11.1Mb sub-directories of 1Mb or more: data 7.2Mb doc 1.0Mb R 2.5Mb R CMD check generated the following check_fail: 1. rcmdcheck_reasonable_installed_size #### Test coverage with [covr](https://covr.r-lib.org/) Package coverage: 97.82 #### Cyclocomplexity with [cyclocomp](https://github.com/MangoTheCat/cyclocomp) No functions have cyclocomplexity >= 15 #### Static code analyses with [lintr](https://github.com/jimhester/lintr) [lintr](https://github.com/jimhester/lintr) found the following 6 potential issues: message | number of times --- | --- Avoid library() and require() calls in packages | 5 unexpected symbol | 1

5. Other Checks

Details of other checks (click to open)

:heavy_multiplication_x: The following 10 function names are duplicated in other packages: - - `aux` from seewave - - `get_code` from norgeo, rmonad, xpose - - `get_data` from canvasXpress.data, cbsodataR, cimir, completejourney, CVXR, danstat, deckgl, ecb, finnishgrid, ggPMX, ggvis, hydroscoper, insight, jtools, mapbayr, metacoder, missCompare, optimall, qrmtools, r4googleads, radiant.data, radous, rbedrock, rchallenge, rsimsum, SWIM, swissparl, tidyLPA, tidySEM, trending, tsmp, ugatsdb, xpose - - `get_priors` from CausalQueries, insight - - `lags` from smooth, tis, TTR - - `mcmc_diagnostics` from bpr, rater, rnmamod - - `obs` from metacoder, observer - - `plot_deltas` from spruce - - `random` from CoOL, decisionSupport, distributions3, gam, gamlss, ggdmc, lidR, messydates, simr, sodium - - `splines` from rpatrec

Package Versions

|package |version | |:--------|:---------| |pkgstats |0.1.1.20 | |pkgcheck |0.1.0.9 | |srr |0.0.1.178 |

Editor-in-Chief Instructions:

Processing may not proceed until the items marked with :heavy_multiplication_x: have been resolved.

mpadge commented 1 year ago

@santikka Thank you for your help debugging and improving our check system. Sorry the checks took so long to appear here.

@adamhsparks Please ignore the failing checks. The duplicated names check will soon be downgraded from a fail to a note. The other check is brand new. @santikka Feel free to modify your repo to describe return values, but do so at your own leisure, and consider all checks as passing for now. Thanks for all the help!

santikka commented 1 year ago

@mpadge No worries, glad to help! I will add the missing return values soon.

santikka commented 1 year ago

@mpadge About the return values: the system does not seem to recognize function aliases. For example in the above, obs and aux are aliases of dynamiteformula and thus have the same return value implicitly. If I explicitly add the return tag to these aliases as well, the resulting documentation looks rather silly with regards to the output value:

Value
A dynamiteformula object.

A dynamiteformula object.

A dynamiteformula object.
adamhsparks commented 1 year ago

@ropensci-review-bot assign @noamross as editor

ropensci-review-bot commented 1 year ago

Assigned! @noamross is now the editor

noamross commented 1 year ago

@ropensci-review-bot seeking reviewers

ropensci-review-bot commented 1 year ago

Please add this badge to the README of your package repository:

[![Status at rOpenSci Software Peer Review](https://badges.ropensci.org/554_status.svg)](https://github.com/ropensci/software-review/issues/554)

Furthermore, if your package does not have a NEWS.md file yet, please create one to capture the changes made during the review process. See https://devguide.ropensci.org/releasing.html#news

noamross commented 1 year ago

@ropensci-review-bot assign @nicholasjclark as reviewer

ropensci-review-bot commented 1 year ago

@nicholasjclark added to the reviewers list. Review due date is 2022-09-27. Thanks @nicholasjclark for accepting to review! Please refer to our reviewer guide.

rOpenSci’s community is our best asset. We aim for reviews to be open, non-adversarial, and focused on improving software quality. Be respectful and kind! See our reviewers guide and code of conduct for more.

ropensci-review-bot commented 1 year ago

@nicholasjclark: If you haven't done so, please fill this form for us to update our reviewers records.

nicholasjclark commented 1 year ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Functionality

Estimated hours spent reviewing: 5

Review Comments

This is a very nicely-documented package that provides a rich set of functions all tailored towards a single purpose or estimating complex panel models and interrogating resulting models. The authors make good arguments as to why the package is needed, how it extends existing software and who the target audience is. I have not used these models personally so I found it a little difficult to understand how I would go about implementing them for my own purposes. On this note I do feel that the planned accompanying papers will be essential for introducing unfamiliar users to the package. Some extra detail in the vignette and readme could help to provide a bit more context about what the parameters mean and how the models can be interpreted, but overall this does not detract from the soundness of the package itself.

The only issue I find in general testing is that I receive a single failure during unit testing:

Error (test-recovery.R:22:3): parameters for the linear regression are recovered as with lm Error in do.call("dynamite", list(dformula = x, data = data, group = group, time = time, debug = list(no_compile = TRUE), ...)): argument “group” is missing, with no default.

All examples pass on my machine, and they are appropriate for demonstrating the functionality of the package. I do however have a few general notes:

There appears to be a typo in the vignette on line 383 due to a missing closing parenthesis (change mutate(invest = inv, firm = factor(firm) to mutate(invest = inv, firm = factor(firm)))

On line 398 in the vignette I receive the following error: unused argument (random_intercept = TRUE)

On line 514 in the vignette, the text states that the output is a two-component list, but in my run I get a dataframe

Some guidelines for contributing could be listed explicitly if the authors would welcome community contributions

The package has quite a few dependencies that make installation a lengthy process. Could any of this be streamlined? Also, the reliance on the newest version of R could be a deterrent for some users. If dplyr and magrittr are used then why is the native pipe necessary?

I see no reason why the authors shouldn’t allow for cmdstanr (in addition to rstan) to be a backend option for sampling. This would allow a broader range of users to work with the package, as Cmdstan is usually ahead of rstan in terms of features (not to mention much better performance on most systems)

The options for updating priors and for generating Stan code and data objects are incredibly useful and will allow users to modify the files to suit their analyses. However some additional annotations of the Stan code would be helpful to give users better familiarity with the programs, as would a worked example to illustrate how the code could be modified and run outside of dynamite. I also find annotations to be a bit lacking throughout most of the functions. Again this doesn’t harm performance or tidiness of the package but it makes it challenging for users (such as myself) who might want to adapt some of the code for their own workflows and packages. For example, what is the purpose of each chain in the long pipe used in the as_draws_df.dynamitefit function? A few simple annotations would clarify what calculations / manipulations are taking place

It is not clear how users can simulate from the prior to help inform model development. This would be a useful addition to vignettes / examples as it is an essential component of a modern Bayesian workflow

mcmc_diagnostics function is very useful for a quick overview of estimator performance

Very impressive and thorough set of unit tests

Each function appears to have a standalone usage that is well documented

Links to the code used for creation of example datasets are excellent tools for demonstrating how users can implement simulations in their own workflows

noamross commented 1 year ago

Thank you for your review, @nicholasjclark! I note you used the non-statistical reviewer template. Would you be able to use the statistical one at https://stats-devguide.ropensci.org/pkgreview.html#pkgrev-template? My apologies, it looks like the one linked above is dead so you probably searched out and found that one.

I am still seeking a second reviewer, @santikka. I suggest limiting updates until you have inputs from both.

ropensci-review-bot commented 1 year ago

:calendar: @nicholasjclark you have 2 days left before the due date for your review (2022-09-27).

santikka commented 1 year ago

Hi @nicholasjclark,

Thank your for the comprehensive review. We agree that the documentation in its current form is still lacking, and as we move toward writing the relevant papers, the situation will gradually improve as a consequence.

We have corrected the error in the unit testing. We have also corrected the errors and typos in the vignette.

While the native pipe is not strictly necessary, we still prefer it over the magrittr pipe because it is future-proof in a sense and is already widely adopted such as in RStudio and in the newest edition of the popular Advanced R book. The native pipe also has zero overhead in performance.

We have managed to drop the dependency on the development versions of rstan and StanHeaders. This only affects the case where the model contains categorical responses, for which the sampling will be much slower with older versions of these packages. A warning is now issued in such instances.

We have added the option to use cmdstanr as a backend for sampling which can be used with argument backend = "cmdstanr" in the dynamite call.

We have added helpful annotations to the more complicated section of the generated Stan code as well as in other parts of the package code itself. Some larger functions such as dynamite itself have been broken down to smaller simpler functions.

We have added an example on how to simulate from the prior predictive distribution to the documentation pages of predict. Note that this is not particularly useful for the complex models of dynamite because as with other flexible time-series models which do not enforce stationarity, the simulated series tend to vary too much and can "explode" with large number of time points.

nicholasjclark commented 1 year ago

Thank you for your review, @nicholasjclark! I note you used the non-statistical reviewer template. Would you be able to use the statistical one at https://stats-devguide.ropensci.org/pkgreview.html#pkgrev-template? My apologies, it looks like the one linked above is dead so you probably searched out and found that one.

I am still seeking a second reviewer, @santikka. I suggest limiting updates until you have inputs from both.

Hi @noamross. Sorry I didn't see your reply until now, would you still like me to use the statistical template now that @santikka has responded to the original review?

noamross commented 1 year ago

@nicholasjclark Yes, please do. Thanks!

noamross commented 1 year ago

@ropensci-review-bot assign @lucymcgowan as reviewer

ropensci-review-bot commented 1 year ago

@lucymcgowan added to the reviewers list. Review due date is 2022-10-20. Thanks @lucymcgowan for accepting to review! Please refer to our reviewer guide.

rOpenSci’s community is our best asset. We aim for reviews to be open, non-adversarial, and focused on improving software quality. Be respectful and kind! See our reviewers guide and code of conduct for more.

ropensci-review-bot commented 1 year ago

@LucyMcGowan: If you haven't done so, please fill this form for us to update our reviewers records.

ropensci-review-bot commented 1 year ago

:calendar: @lucymcgowan you have 2 days left before the due date for your review (2022-10-20).

santikka commented 1 year ago

Hi @noamross! Would it be possible to get a status update regarding the review process? It has now been over one month since the second review deadline.

LucyMcGowan commented 1 year ago

Im so sorry I’m so behind! Hoping to wrap this up soon

LucyMcGowan commented 1 year ago

Package Review

Compliance with Standards

The authors identify 42 standards that they deem "Non-applicable". I agree with these designations, the majority describe methods that are out of scope for the project or not appropriate given the methods.

The authors complied with the remaining 129 standards. In particular, the documentation and testing in this package is very thorough.

For packages aiming for silver or gold badges:

General Review

Documentation

The package includes all the following forms of documentation:

I did find one TODO left in the lfactor documentation (#TODO definition and constraint on lambdas) that maybe was meant to be updated?

Algorithms

The algorithms are encoded well. The language is appropriate, and all tests passed on my machine.

Testing

This package has extensive testing and all tests passed on my machine.

The examples and vignette, however, are not currently compiling, for example, when I run the first example of the dynamite function I get the following error:

library(dynamite)
fit <- dynamite(
       dformula = obs(y ~ -1 + varying(~x), family = "gaussian") +
         lags(type = "varying") +
         splines(df = 20), gaussian_example, "id", "time",
       chains = 1,
       refresh = 0
     )
#> Error in `[.data.table`(data, idx, cl, env = list(cl = cl)): unused argument (env = list(cl = cl))

Created on 2022-12-06 by the reprex package (v2.0.1)

EDIT updating to the development version of the data.table package fixed this, however in the README when I tried to run data.table::update.dev.pkg() I got the following message: Error: 'update.dev.pkg' is not an exported object from 'namespace:data.table'.

Visualisation (where appropriate)

Package Design

Estimated hours spent reviewing: 4

santikka commented 1 year ago

@LucyMcGowan Thank you for the review!

Regarding the error, dynamite currently requires the development version of the data.table package (which can be installed via data.table::update.dev.pkg()). There is some confusion about the version numbers, since the required version 1.14.3 of data.table is already on CRAN, but the particular feature used by dynamite was not included in the CRAN release: https://github.com/Rdatatable/data.table/issues/5538 we will update the readme accordingly to avoid confusion.

nicholasjclark commented 1 year ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Compliance with Standards

The following standards currently deemed non-applicable (through tags of @srrstatsNA) could potentially be applied to future versions of this software: (Please specify)

Please also comment on any standards which you consider either particularly well, or insufficiently, documented.

For packages aiming for silver or gold badges:

The authors have been able to comply with 129 standards with only 42 N/A standards across the “Bayesian and Monte Carlo” and “Regression and Supervised Learning” categories and the function design is very well compartmentalised in my view

General Review

Documentation

The package includes all the following forms of documentation:

The following sections of this template include questions intended to be used as guides to provide general, descriptive responses. Please remove this, and any subsequent lines that are not relevant or necessary for your final review.

Algorithms

As the maintainers have now allowed for cmdstanr to be used as a backend, any efficiency updates can generally be automatically incorporated as long as users keep their Cmdstan and cmdstanr packages up to date. Perhaps some instructions to the users in the Readme can help to clarify this

Testing

Very impressive and thorough set of unit tests

Visualisation (where appropriate)

Visualisations make use of ggplot objects, which are very familiar to most potential users. There is no risk of misinterpretation in my view

Package Design

Function designs and levels of documentation are excellent and the returned structures will integrate very well with popular post-processing packages designed to work with stanfit objects. The algorithms make use of stan, which is the most efficient implementation of MCMC in the R programming environment. I see no issues with the way the package is designed

Estimated hours spent reviewing: 5

LucyMcGowan commented 1 year ago

Thank you! I updated to the dev version of data.table and was able to compile everything. I did try to follow the instructions in the README by running data.table::update.dev.pkg() but got the following message: Error: 'update.dev.pkg' is not an exported object from 'namespace:data.table'. I have updated my review above. Thank you!

santikka commented 1 year ago

@LucyMcGowan It seems that at some point the development version installation function of data.table was renamed into update_dev_pkg instead, apologies! The README has been corrected accordingly.

santikka commented 1 year ago

Based on the reviews, we have taken some additional steps to make the package installation more straightforward and the requirements less strict:

noamross commented 1 year ago

@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/554#issuecomment-1341808955 time 5

noamross commented 1 year ago

@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/554#issuecomment-1339614792 time 4

noamross commented 1 year ago

Thank you all for bearing with us and following up! @LucyMcGowan and @nicholasjclark, you seem to have indicated in your follow-ups, but to be unambiguous, do the updates resolve any outstanding issues for acceptance at gold level?

LucyMcGowan commented 1 year ago

Yes, I approve acceptance at the gold level. Thanks @noamross!

nicholasjclark commented 1 year ago

Thank you all for bearing with us and following up! @LucyMcGowan and @nicholasjclark, you seem to have indicated in your follow-ups, but to be unambiguous, do the updates resolve any outstanding issues for acceptance at gold level?

Yes I approve acceptance as well. Thanks very much

noamross commented 1 year ago

@ropensci-review-bot approve dynamite

ropensci-review-bot commented 1 year ago

Approved! Thanks @santikka for submitting and @nicholasjclark, @lucymcgowan for your reviews! :grin:

To-dos:

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent).

Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @ropensci/blog-editors in your reply. They will get in touch about timing and can answer any questions.

We maintain an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding (with advice on releases, package marketing, GitHub grooming); the guide also feature CRAN gotchas. Please tell us what could be improved.

Last but not least, you can volunteer as a reviewer via filling a short form.

santikka commented 1 year ago

@ropensci-review-bot finalize transfer of dynamite

ropensci-review-bot commented 1 year ago

Transfer completed. The dynamite team is now owner of the repository and the author has been invited to the team