JGCRI/ambrosia

Hollister JOSS Review

Closed this issue · 0 comments

jhollist commented

Below find my review comments for the JOSS review at openjournals/joss-reviews#2890. Any questions, please let me know.

General comments

Thank you for the chance to review this package. Subject matter is mostly outside of my area of expertise and I did learn quite a bit. That being said, anytime software can be written to help ease implement important modelling efforts, then that is worthy of sharing. Your ambrosia package is no exception! Nicely done.

A couple of other general comments:

  1. I like the shiny app. I am mostly ignorant of food demand models so having this helped me.
  2. Your documentation is very extensive and you provide a detailed vignette. This will be appreciated by your users.

Below I have provide some comments related to the items on the checklist that I have not yet checked off on. I have also included some specific comments for you to consider in your revisions. Nothing major, just some items that I think will help improve upon an already good contribution.

Checklist comments

General Checks: Contribution and authorship:

  • Robert Link (rplzz) had the most commits to repo (granted they are a bit old) but not listed as an author on the paper or the package. Also authors James Edmunds and Ryna Cui did not commit (not questioning their inclusion as many other contributions besides commits!). Just take a second look at authors to make sure everyone is good!

Functionality: Installation:

  • In README's installation section think about using remotes::install_github() instead of devtools::install_github(). remotes is much lighter weight and focuses on the install which is the specific use case here. There is some disagreement on this but I prefer remotes. Again, up to you all.

  • A few Rd warnings on install. First three are from including () in the \link{}. Last one appears to be from line 172 in food-demand-mc.R. Nothing to link to in those.

Rd warning: C:/Users/JHollist/AppData/Local/Temp/RtmpOSX3FJ/R.INSTALL516c76db875/ambrosia/man/calculate_ambrosia_params.Rd:5: missing link 'create_dataset_for_parameter_fit()'

Rd warning: C:/Users/JHollist/AppData/Local/Temp/RtmpOSX3FJ/R.INSTALL516c76db875/ambrosia/man/calculate_ambrosia_params.Rd:59: missing link 'create_dataset_for_parameter_fit()'

Rd warning: C:/Users/JHollist/AppData/Local/Temp/RtmpOSX3FJ/R.INSTALL516c76db875/ambrosia/man/calculate_ambrosia_params.Rd:23: missing link 'create_dataset_for_parameter_fit()'

Rd warning: C:/Users/JHollist/AppData/Local/Temp/RtmpOSX3FJ/R.INSTALL516c76db875/ambrosia/man/vec2param.Rd:69: missing link 'A_s, A_n, xi_ss, xi_cross, xi_nn, nu1_n, lambda_s, k_s, Pm, psscl, pnscl'

Documentation: A statement of need:

  • Need a bit more on this in the README. Target Audience? Why is this needed as an R package (or as any software solution)?
  • A struggle I always have with JOSS reviews is the distinction between what is in the repository documentation (e.g. README) and what should go in the paper. Summary's in this case are the same and Statement of need is in the paper but not currently in the README. I personally think having them repeated is OK, because I think you are getting this information to people at different times with the different formats so some repeat is good. That being said, a little clarification from editor is probably warranted.

Software Paper: References:

Specific comments

  • Even though this is not necessarily headed to CRAN, I would make sure that all of the CRAN checks pass. On my machine I had 5 warnings and 4 notes when I ran CRAN checks.
  • Vignette Examples: Stick with left assign on on the examples. You have a few cases where you use it. I do it too, but for examples may be best to stick to the traditional left assign.
  • Vignette Example 3.1: Needs shiny installed in order to run. Make note for users that may not have shiny already installed.
  • Vignette Example 4.4: Extra "," in calculate_ambrosia_params?
  • Vignette Example 5.1: Also long running. Add note to vignette. Also any way to speed up/reduce runtime of examples? Took 50+ on my laptop
  • Vignette Example 5.1: Extra "," in calculate_ambrosia_params?
  • clean up actions. Build and r-cmd-check appear to be doing mostly the same thing (with build sending test coverage) and both are macOS with old versions of R. Minimum, in rdmd.yml add in a windows build on release and bump mac0S R version in both yml to release.
matrix:
        config:
          - {os: windows-latest, r: 'release'}
          - {os: macos-10.15, r: 'release'}
  • Function names use both dot case and snake case. Having different naming conventions for the functions in the package will make it a challenge for your users. Pick one and stick with it.
  • Paper: line 57: "of goods" should it be "of goods: "
  • Paper: line 87: No need to refer directly to an rmd here. Link text could simply be "ambrosia vignette" the links to the html.
  • Paper: line 107 "User" should be "Users"