JOSS review

[amoeba]

Submission: https://github.com/openjournals/joss-reviews/issues/3118 Status: Conditional accept

Hey @aghaynes & @majensen,

I've finished my review and was able to check off most items but a few remain. The submission is in very good shape overall and I think the changes are minor.

First off, kudos @aghaynes to you and your team for producing such a nicely put together package. Your description of the background for this work and your accompanying documentation made it easy for me to get familiar with the technique and explore the functionality of the package. The documentation looked very complete and the Shiny app is nice touch, especially in the way it automatically generates R code to match.

Below are the items I'd like to see addressed before I can change my review to an Accept:

Contribution and authorship

I see different sets of authors in a few places and wanted to make sure everything was complete and correct:

• Paper: Haynes, Lenz, Stalder, Limacher
• R package: Lenz [aut], Haynes [cre,aut], Limacher [aut], Stalder [ctb]
• git commit history: Haynes, Lenz, Stalder (by order of commits)
• pkgdown site: Lenz, Haynes, Limacher

First, could you clarify Limacher's association and contribution to the package? Second, is the above information (including ordering) correct?

Functionality

Everything worked great except the Shiny app. When I install the package, load the package, and attempt to run the Shiny app, I get an error and the app does not launch:

Error: In sliderInput(): min, max, and value cannot be NULL, NA, or empty. 24. stop(call. = FALSE, sprintf("In %s(): min, max, and value cannot be NULL, NA, or empty.", fun)) 23.

I had a hunch and checked to see if the Shiny app worked under the development version and it does. I assume the authors are aware of this but I think it warrants ensuring JOSS archives a fixed version of the software and not 0.1.3.

Let me know if you can reproduce my issue. If so, I think generating a new release and updating this submission is warranted.

Community guidelines

Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

I wasn't able to find any of these three bits of information in the README file. Please add them.

Paper

The paper is overall well-written and quite useful. However, I believe it doesn't fit in with the JOSS guidelines in two ways:

1. It does not contain an initial, high-level summary paragraph entitled Summary
2. It contains too much detail about the package. That said, the information that falls outside the scope of the JOSS paper is very useful information and could be moved elsewhere

Other, non-review details

These are outside the JOSS guidelines so the review is not contingent on addressing them:

• You might link your pkg site, https://ctu-bern.github.io/presize from GitHub (and possibly other places)
• There's a typo at the bottom of the Example section in the README, "THe" instead of "The"
• The image at the bottom of your pkgdown site under the Funding header isn't working for me

• I noticed some scenarios where passing nonsense output produces cryptic errors both when calling functions directly or calling them indirectly using the Shiny app. For example, if I mistakenly specified the conf.width argument in percent points (e.g., 5) rather than as a proportion (e.g., 0.05),

  prec_prop(p = 0.5, conf.width = 5, conf.level = 0.95, method = 'wilson')
  Error in uniroot(function(n) eval(quo) - prec, c(1, 1e+07), ...) :
  f() values at end points not of opposite sign

  or in the Shiny app:

  Error: An error has occurred. Check your logs or contact the app author for clarification.

  I don't think these are deal breakers and I know writing tons of validation logic can be time-consuming but I thought I'd point out that I ran into it.

Please let me know if you have any questions of if I can clarify anything.

[TomKellyGenetics]

Just a quick note here (I'll submit my review later): Shiny version 1.6.0 appears to be available on CRAN and this issue has been fixed in version 1.4.0. Is this an issue with the package itself or missing dependencies?

Update: I've tested this on my system and it works on Shiny v1.5.0 (and 1.4.0). There appears to be a bug with Shiny version 1.6.0 and GitHub rstudio "master" versions as well.

> library("shiny")
> library("presize")
> launch_presize_app()

Listening on http://127.0.0.1:5203
Warning: Error in isTRUE: object 'lang' not found
  [No stack trace available]

As a workaround this appears to work if there is no default locale in the R environment:

> lang="UTF8"
> launch_presize_app()

This doesn't seem to be the fault of the authors (although I recommend testing with the latest version of dependencies) but it could be avoided from users having older versions by depending on shiny (>= 1.7.0) when it becomes available.

[aghaynes]

I was going to respond to the whole of @amoeba's review in one go once I was done addressing the comments, but as @TomKellyGenetics has brought up the shiny issue, I'll address that now...

Firstly, thank you both for agreeing to review the paper and package! We really appreciate it!

So, onto Shiny...

The issue that @amoeba brought up in the functionality section regarding an error in "min/max/value cannot be NULL" appears to be due to a change in the Shiny defaults to sliderInput. As @amoeba pointed out, I discovered that recently and have resolved the problem by setting a default of 0.5 on the sliderInputs in question (I also added reset buttons for the N and CI width on each page of the page). This is all in the dev version (currently v0.2.1, but I will probably increment the version again with any additional points from you both).

I've not come across the issue that @TomKellyGenetics mentioned... on either an older version of R and packages nor on more up to date versions... i guess i must have a default locale set.

In any case, I will look into testing the shinyapp with the shinytest package which can be integrated into the testthat and continuous integration framework we already use.

While I'm writing, @amoeba noted that the figure at the bottom of the pkgdown site doesn't work. You're correct. I've tried a number of things (e.g. different formats) and nothing seems to work. It appears to me that pkgdown isnt copying the figure from the folder to the fork used for the pkgdown site. Do either of you have any idea about that?

As I say, I'll reply to other points later...

[TomKellyGenetics]

Thanks for responding so quickly @aghaynes. It makes sense to incorporate changes to address reviews before submitting a new release to CRAN and incrementing the version number associated with the JOSS manuscript to reflect this. On closer reading, this seems to be what @amoeba was suggesting (rather than referring to Shiny versions) as the dev version already solves the missing default values.

I'll test the shiny app (which is a great idea to include!) more and may submit a separate issue if my locale issues persist.

[aghaynes]

Once again, thanks for reviewing @amoeba. Here's our response your points. The changes are currently in PR #64 and will hopefully be merged today, once the extra validations are done (this is perhaps relevant to @TomKellyGenetics... maybe you want to review that version?).

contribution and authorship

• paper

  • Haynes and Limacher wrote the paper.
  • Stalder put a not-insignificant amount of work into presize testing and we feel that she is thus entitled to coauthorship.
  • Stalder and Lenz provided varying degrees of feedback.

• R package/git commit history

  • The initial idea for the package came from Limacher, who also secured the funding from the SCTO. Without him, the package probably wouldnt exist.
  • Lenz was the lead package developer until he left CTU ca two years ago.
  • Haynes then took up the lead developer position.
  • Limacher has provided code snippets directly to Lenz or Haynes (sometimes in R, sometimes in STATA), but not written code specifically for the package. Limacher also tested functionality throughout the development process.
  • Stalder was a great help in providing support in testing the package against other known approaches but has not authored any of the functions (hence the ctb flag)

• pkgdown

  • pkgdown defines authors based on info in the DESCRIPTION file.
  • pkgdown only displays those indicated as authors (with the au flag) at the foot of the page, hence only Lenz, Haynes and Limacher are shown.
  • There is a developers section on the right hand side of the page. Clicking this takes you to the "authors" page which details all contributors (au, ctb, any other flags from the DESCRIPTION file).
  • Thus, although not immediately visible, Stalder is given credit on the pkgdown site too.

Although Limacher has not authored code in the package (and thus does not appear in the git commit history), he has been instrumental in its development.

As far as we are concerned, the author lists are correct and appropriate.

functionality

As already discussed, I found the issue with Shiny recently and fixed it. As far as I can tell, this was due to change in the defaults for the functions in the shiny package.

I absolutely agree, the version of the package mentioned in JOSS should have this issue fixed (which it has been in version 0.2.1). Once any other issues are resolved, I will make a release, submit a new version to CRAN and to e.g. zenodo for archiving, as per JOSS requirements.

@majensen, do I need to do anything to tell whedon to use a different version or does it focus on the head version?

community guidelines

Thanks for pointing this out. I have added sections "Getting help" and "Contributing" to the readme (which are then percolated through to the pkgdown site automatically).

paper

We added a short summary section at the beginning of the paper.

We have removed most of the "usage" section and the table detailing the different methods to the readme file. We left a small example in the paper, however.

non-review details

link to the pkgdown site

• this is a great idea! We have added links to both the readme and the shinyapp.

typo (THe)

• thanks! fixed.

image missing on the pkgdown site

• this should be fixed now.

nonsense input results in cryptic errors

• thanks for pointing this out. We already had a fair amount of validation and are adding more, to basically every function, which should robustify the app/package against that particular issue. We're still looking at this though.

[TomKellyGenetics]

@aghaynes Thanks for letting me know. I was planning to review today as well but I will check this and avoid repeating issues already raised here (or addressed by your PR).

[amoeba]

Sorry for the delayed response @aghaynes. Thanks for taking the time to work over my review and make the changes. Extra thanks for making those changes in a PR so I can check them out easily.

• Re: paper/authorship: That sounds great to me. Thanks for laying that out in so much detail and thanks for confirming things are correct.
• functionality: 👍
• community guidelines: 👍
• non-review details: 👍

• paper: Thanks, those changes improve things. The paper reads very well, is nicely-written, and I'd sign on off it except for this section from the Review criteria:

  Note the paper should not include software documentation such as API (Application Programming Interface) functionality, as this should be outlined in the software documentation.

  I think the Usage section violates the intent of this rule. Instead of guessing, maybe we could pull in @majensen.

The only outstanding part of my review is the above question about the paper. Let's see what @majensen says and go from there?

[aghaynes]

Thanks @amoeba. I had a quick look at a few other R papers in JOSS to get a feel for what would be OK and there are examples with code in them (sometimes much more than in this paper)... Happy for @majensen to comment though

[TomKellyGenetics]

Regarding this point, @majensen previously edited this paper of mine which also includes R code. I agree that the examples in the documentation should stand on their own but usage sections in R papers often follow a "vignette" style like this.

PR #64 seems to pass CI checks so if it is merged I'm available to review the updated manuscript.

[aghaynes]

Hi both, I've merged #64 into the main branch. The shiny app online takes a little more effort to update... thats something for tomorrow or thursday I think.

[majensen]

Hi @amoeba @TomKellyGenetics @aghaynes -- my feeling about R is that code in the docs is part of its culture. It's natural to give an example in the paper, it's not "documentation" per se. So unless I'm overridden by a higher power, I extend to you the scepter of approval.

[amoeba]

Sounds good to me, thanks very much @majensen. My review is now an Accept.

[aghaynes]

Thanks @amoeba! :)