JOSS Review

[ezraporter]

Thanks for allowing me to review https://github.com/openjournals/joss-reviews/issues/6971! Below are some comments that address the unchecked boxes on my checklist.

Installation

• The package doesn't provide an automated method for installing the JAGS dependency. The R ecosystem seems to have neglected this generally so it seems unfair to hold this package responsible for writing a suitable installation script. Minimally, the README should include a link out to where JAGS came be downloaded or instructions like these: https://gist.github.com/dennisprangle/e26923fae7477566510757ab3341f54c.

Documentation

• Documentation for BJSM_binary() says n_MCMC_chain defaults to 1 but it has no default.
• Documentation for LPJSM_binary() says ... are not currently used but they appear to be forwarded to geepack::geeglm()

Manuscript

• The statement of need makes specific reference to trial design and power calculation utilities. However, these are only discussed in the manuscript in Table 1. The manuscript could benefit from more detailed information about what these utilities are and why they'd be useful for a trial.
• The manuscript contains a good amount of API documentation and example output that's already available in the package documentation.

Additional suggestions not impacting the review

• Naming conventions differ across the input data for BJSM_binary(), group_seq(), and BJSM_c(). Harmonizing them might make for a cleaner user experience. For example, these sets of values all seem to contain similar information but have different names:
  • treatment_stageI/response_stageI/treatment_stageII/response_stageII
  • trt.1st/resp.1st/trt.2nd/resp.2nd
  • trt1/stage1outcome/trt2/stage2outcome
• Some parameters are mapped directly to parameters of lower-level functions like rjags::jags.model. It would be helpful to note in the documentation where this is happening so the user can know where to get more information.
• The output of BJSM_binary() might be improved by grouping together common elements into a nested list. For example, there are many elements prefixed with ci_. Could those be better collected into a ci element where you could access ci$pi_A and ci$pi_B. My general advice would be to look at where you have underscores and consider if that's better as a nested list.

[sidiwang]

Hi @ezraporter,

Thank you very much for your valuable suggestions. I have reviewed them one by one and made edits according to every bullet point you listed under "Installation," "Documentation," and "Manuscript." Could you please take a look at the repository again and let me know if you feel further changes are needed? Regarding the "Additional suggestions not impacting the review," I will make those changes in the next few weeks.

Thanks, Sidi

[ezraporter]

Thanks! I'll have a chance to review tomorrow and will report back!

[ezraporter]

Thanks for those updates! All looks good to me and I've noted it in the review issue 😊

[sidiwang]

@ezraporter Thank you!