JOSS Review - Githubissues

These are some notes for the JOSS paper review. In general, I think it's a nice paper and potentially useful package.

Comments regarding the paper:

A quick question regarding authorship: I noticed that three paper authors don't seem to have a connection with the software, while one contributor to the software isn't included as an author, despite contributing over 1000 lines of new code...?
Datasets: consider indicating whether the datasets you include are synthetic or real. If they are real, you should cite the sources.
Figure 1: consider adding a caption to the figure so that it's clear what is shown.
Example usage: I wonder whether showing and discussing the result of the model would be helpful...?

Specific to the package itself:

Contributing guidelines: You dont seem to have any guidance on how users can contribute.
As your package depends on jags, perhaps add it as a system dependency to your DESCRIPTION (see e.g. rjags)
The helpfile for summary.BJSM_binary speaks of "Expected Response Rate of Dynamic Treatment Regimens (DTR)" being a value in the output, but i dont see that anywhere (neither in the print nor the object itself)

The summary method for BJSM_c objects is less clear than that for BJSM_binary (e.g. V1[1,1]??). Perhaps you can harmonise the summary methods a little (e.g. split the continuous one into three matrices with informative headers as per the binary case)?


# from the helpfile
BJSM_result <- BJSM_c(
data = trialData, xi_prior.mean = c(50, 50, 50),
xi_prior.sd = c(50, 50, 50), phi3_prior.sd = 20, n_MCMC_chain = 1,
n.adapt = 1000, MCMC_SAMPLE = 5000, BURIN.IN = 1000, ci = 0.95, n.digits = 5, verbose = FALSE
)
summary(BJSM_result)

Parameter Estimation: Estimate CI CI_low CIhigh V1[1,1] 111.63507 0.95 6.070278e+01 168.7209463 V1[2,1] 85.64805 0.95 4.870137e+01 134.8992087 V1[1,2] 85.64805 0.95 4.870137e+01 134.8992087 V1[2,2] 77.70400 0.95 4.277722e+01 119.0750054 V2[1,1] 123.18354 0.95 6.562162e+01 194.7928189 V2[2,1] 29.16360 0.95 -2.018850e+01 77.8351264 V2[1,2] 29.16360 0.95 -2.018850e+01 77.8351264 V2[2,2] 107.75252 0.95 5.664411e+01 173.8732619 phi1 0.18433 0.95 1.234352e-05 0.3853457 phi3 4.00328 0.95 2.654285e+00 5.2424762 xi[A] 51.10907 0.95 4.725783e+01 54.8613873 xi[B] 62.04274 0.95 5.842472e+01 66.1343026 xi[C] 69.05152 0.95 6.529155e+01 72.7509413


(I've just looked at the Hartman paper, and see where the V1 and V2s are coming from, but it would be helpful to have more information in the helpfiles rather than just "see the paper in the references")
- Where you have references in helpfiles, it would be useful to add DOIs and perhaps even hyperlinks

Places for possible improvement that won't influence the review itself
- I would encourage you to set up a pkgdown site, and add the link to your repo settings and description file. It looks like you already had a github action doing part of it (you have a gh-pages branch). Vignettes are also often very helpful and would allow you to expand on your documentation beyond what is possible in the individual helpfiles. 
- Testing: You use the testthat package, which is great. I would encourage you to give more meaningful names to your tests (rather than e.g. "BJSM_binary 6"). It looks to me like you may only be testing a very small fraction of the output from your objects (`pi_hat_bjsm`). It may be worth testing more components... Further, you could split your tests into multiple files (one per function tested gives a good oversight of whats been tested and what hasn't)

Thank you very much for your feedback!

Regarding the authorship, I will discuss this with all the authors and get back to you.

The datasets are synthetic; I will add this to the descriptions.

I will make changes according to all the other points you mentioned asap and will let you know when they are done. Just out of curiosity, is there a deadline for this?

Thanks again for reviewing the paper.

You're welcome. If you have any questions, let me know 😄

Re the review, thats a little unclear to me too, although the editorbot said "please make sure to complete your review within six weeks" in the review issue. Whether that includes your response/changes I don't know.

Hi @aghaynes,

Thank you very much for all your valuable suggestions. Please find my responses to your comments below. Please let me know if more edits are needed! :)

These are some notes for the JOSS paper review. In general, I think it's a nice paper and potentially useful package.

Comments regarding the paper:

A quick question regarding authorship: I noticed that three paper authors don't seem to have a connection with the software, while one contributor to the software isn't included as an author, despite contributing over 1000 lines of new code...? I have included Mike in the acknowledgments because he only helped with publishing the R package on CRAN and didn't contribute to the statistical method in any way. Although the three paper authors didn't contribute to the software, they contributed to the statistical methods, which is a very important part of the package.

Datasets: consider indicating whether the datasets you include are synthetic or real. If they are real, you should cite the sources. In the descriptions of the datasets, I added a note that the datasets are synthetic.

Figure 1: consider adding a caption to the figure so that it's clear what is shown. Thank you for the suggestion. I added a caption.

Example usage: I wonder whether showing and discussing the result of the model would be helpful...? Thank you for the suggestion. I added some explanations.

Specific to the package itself:

Contributing guidelines: You dont seem to have any guidance on how users can contribute. I added a file named "CONTRIBUTING.md" in the ".github" folder. I'm not 100% sure if this is the correct way to add it. If more details need to be added, please let me know.

As your package depends on jags, perhaps add it as a system dependency to your DESCRIPTION (see e.g. rjags) The DESCRIPTION file currently lists JAGS under 'SystemRequirements:'. Is it fine to leave it this way?

The helpfile for summary.BJSM_binary speaks of "Expected Response Rate of Dynamic Treatment Regimens (DTR)" being a value in the output, but i dont see that anywhere (neither in the print nor the object itself) _This is because, in the example I provided, I set DTR = FALSE in the function call. If we set DTR = TRUE, the DTR will be included in the output. To avoid confusion, I added 'only when DTR = TRUE' to the help file for summary.BJSM_binary._

The summary method for BJSM_c objects is less clear than that for BJSM_binary (e.g. V1[1,1]??). Perhaps you can harmonise the summary methods a little (e.g. split the continuous one into three matrices with informative headers as per the binary case)?
# from the helpfile
BJSM_result <- BJSM_c(
    data = trialData, xi_prior.mean = c(50, 50, 50),
    xi_prior.sd = c(50, 50, 50), phi3_prior.sd = 20, n_MCMC_chain = 1,
    n.adapt = 1000, MCMC_SAMPLE = 5000, BURIN.IN = 1000, ci = 0.95, n.digits = 5, verbose = FALSE
)
summary(BJSM_result)

Parameter Estimation:
         Estimate   CI        CI_low     CI_high
V1[1,1] 111.63507 0.95  6.070278e+01 168.7209463
V1[2,1]  85.64805 0.95  4.870137e+01 134.8992087
V1[1,2]  85.64805 0.95  4.870137e+01 134.8992087
V1[2,2]  77.70400 0.95  4.277722e+01 119.0750054
V2[1,1] 123.18354 0.95  6.562162e+01 194.7928189
V2[2,1]  29.16360 0.95 -2.018850e+01  77.8351264
V2[1,2]  29.16360 0.95 -2.018850e+01  77.8351264
V2[2,2] 107.75252 0.95  5.664411e+01 173.8732619
phi1      0.18433 0.95  1.234352e-05   0.3853457
phi3      4.00328 0.95  2.654285e+00   5.2424762
xi_[A]   51.10907 0.95  4.725783e+01  54.8613873
xi_[B]   62.04274 0.95  5.842472e+01  66.1343026
xi_[C]   69.05152 0.95  6.529155e+01  72.7509413
(I've just looked at the Hartman paper, and see where the V1 and V2s are coming from, but it would be helpful to have more information in the helpfiles rather than just "see the paper in the references") _Thank you for bringing this to my attention. In the help file for BJSM_c, I clarified what V1 and V2 are in the 'Value' section. Please let me know if additional information is needed._

Where you have references in helpfiles, it would be useful to add DOIs and perhaps even hyperlinks Thank you for pointing this out. I added DOIs to all the references.

Places for possible improvement that won't influence the review itself Thank you for the suggestions! I will work on modifying the testing and setting up a pkgdown site in the next few weeks.

I would encourage you to set up a pkgdown site, and add the link to your repo settings and description file. It looks like you already had a github action doing part of it (you have a gh-pages branch). Vignettes are also often very helpful and would allow you to expand on your documentation beyond what is possible in the individual helpfiles.

Testing: You use the testthat package, which is great. I would encourage you to give more meaningful names to your tests (rather than e.g. "BJSM_binary 6"). It looks to me like you may only be testing a very small fraction of the output from your objects (pi_hat_bjsm). It may be worth testing more components... Further, you could split your tests into multiple files (one per function tested gives a good oversight of whats been tested and what hasn't)

Hi @sidiwang

Thanks for addressing the points. Just a couple of very minor comments below.

Hi @aghaynes,

Thank you very much for all your valuable suggestions. Please find my responses to your comments below. Please let me know if more edits are needed! :)

You're most welcome

These are some notes for the https://github.com/openjournals/joss-reviews/issues/6971. In general, I think it's a nice paper and potentially useful package.

Comments regarding the paper:

A quick question regarding authorship: I noticed that three paper authors don't seem to have a connection with the software, while one contributor to the software isn't included as an author, despite contributing over 1000 lines of new code...? I have included Mike in the acknowledgments because he only helped with publishing the R package on CRAN and didn't contribute to the statistical method in any way. Although the three paper authors didn't contribute to the software, they contributed to the statistical methods, which is a very important part of the package.

Thanks for clarifying. Then it's fine.

Datasets: consider indicating whether the datasets you include are synthetic or real. If they are real, you should cite the sources. In the descriptions of the datasets, I added a note that the datasets are synthetic.

👍

Figure 1: consider adding a caption to the figure so that it's clear what is shown. Thank you for the suggestion. I added a caption.

Looks good.

Example usage: I wonder whether showing and discussing the result of the model would be helpful...? Thank you for the suggestion. I added some explanations.

Thanks, thats a bit easier to follow. As there's rounding, perhaps it's worth referring to the column names?

Specific to the package itself:

Contributing guidelines: You dont seem to have any guidance on how users can contribute. I added a file named "CONTRIBUTING.md" in the ".github" folder. I'm not 100% sure if this is the correct way to add it. If more details need to be added, please let me know.

The contributing.md should be at the top level. You just need to add it to the .Rbuildignore file. The content of the file looks fine. Once you've moved it, i can check the last checkbox in the review :smile:

As your package depends on jags, perhaps add it as a system dependency to your DESCRIPTION (see e.g. rjags) The DESCRIPTION file currently lists JAGS under 'SystemRequirements:'. Is it fine to leave it this way? The helpfile for summary.BJSM_binary speaks of "Expected Response Rate of Dynamic Treatment Regimens (DTR)" being a value in the output, but i dont see that anywhere (neither in the print nor the object itself)

This is because, in the example I provided, I set DTR = FALSE in the function call. If we set DTR = TRUE, the DTR will be included in the output. To avoid confusion, I added 'only when DTR = TRUE' to the help file for summary.BJSM_binary.

👍

The summary method for BJSM_c objects is less clear than that for BJSM_binary (e.g. V1[1,1]??). Perhaps you can harmonise the summary methods a little (e.g. split the continuous one into three matrices with informative headers as per the binary case)?

# from the helpfile
BJSM_result <- BJSM_c(
    data = trialData, xi_prior.mean = c(50, 50, 50),
    xi_prior.sd = c(50, 50, 50), phi3_prior.sd = 20, n_MCMC_chain = 1,
    n.adapt = 1000, MCMC_SAMPLE = 5000, BURIN.IN = 1000, ci = 0.95, n.digits = 5, verbose = FALSE
)
summary(BJSM_result)

Parameter Estimation:
         Estimate   CI        CI_low     CI_high
V1[1,1] 111.63507 0.95  6.070278e+01 168.7209463
V1[2,1]  85.64805 0.95  4.870137e+01 134.8992087
V1[1,2]  85.64805 0.95  4.870137e+01 134.8992087
V1[2,2]  77.70400 0.95  4.277722e+01 119.0750054
V2[1,1] 123.18354 0.95  6.562162e+01 194.7928189
V2[2,1]  29.16360 0.95 -2.018850e+01  77.8351264
V2[1,2]  29.16360 0.95 -2.018850e+01  77.8351264
V2[2,2] 107.75252 0.95  5.664411e+01 173.8732619
phi1      0.18433 0.95  1.234352e-05   0.3853457
phi3      4.00328 0.95  2.654285e+00   5.2424762
xi_[A]   51.10907 0.95  4.725783e+01  54.8613873
xi_[B]   62.04274 0.95  5.842472e+01  66.1343026
xi_[C]   69.05152 0.95  6.529155e+01  72.7509413

(I've just looked at the Hartman paper, and see where the V1 and V2s are coming from, but it would be helpful to have more information in the helpfiles rather than just "see the paper in the references") Thank you for bringing this to my attention. In the help file for BJSM_c, I clarified what V1 and V2 are in the 'Value' section. Please let me know if additional information is needed.

Thats clearer, thanks!

Where you have references in helpfiles, it would be useful to add DOIs and perhaps even hyperlinks Thank you for pointing this out. I added DOIs to all the references.

👍

Places for possible improvement that won't influence the review itself Thank you for the suggestions! I will work on modifying the testing and setting up a pkgdown site in the next few weeks.

👍

I would encourage you to set up a pkgdown site, and add the link to your repo settings and description file. It looks like you already had a github action doing part of it (you have a gh-pages branch). Vignettes are also often very helpful and would allow you to expand on your documentation beyond what is possible in the individual helpfiles. Testing: You use the testthat package, which is great. I would encourage you to give more meaningful names to your tests (rather than e.g. "BJSM_binary 6"). It looks to me like you may only be testing a very small fraction of the output from your objects (pi_hat_bjsm). It may be worth testing more components... Further, you could split your tests into multiple files (one per function tested gives a good oversight of whats been tested and what hasn't)

Hi @aghaynes,

Thank you for the feedback! I have moved the contributing.md file, added it to the .Rbuildignore file, and added row names to the result discussions. I hope they look better now!

Thanks again for your review.

Regards, Sidi

Hi @sidiwang I just noticed that you mention a contributor code of conduct in your contributing.md, but the file doesnt seem to be there... it would also be fine to put everything in one file in my opinion.

Hi @aghaynes,

Thank you! I forgot to include that file. Just added it. Please check the updated JOSS branch. :)

Thanks, Sidi

sidiwang / snSMART

JOSS Review #23