JOSS review about presize HOT 12 CLOSED

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.

from presize.

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...

from presize.

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

from presize.

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.

from presize.

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.

from presize.

@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).

from presize.

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?

from presize.

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

from presize.

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.

from presize.

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.

from presize.

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.

from presize.

Thanks @amoeba! :)

from presize.