JOSS Review Notes (Hao Ye, @ha0ye)

[ha0ye]

I'm collecting all the details for my review in this one issue. @mcsiple or another maintainer may consider splitting specific action items into their own separate issues for better task management.

Note: many of these are suggestions for improvements, and in some cases are not actual obstacles for passing the review process at JOSS. These items are indicated by [~] rather than an empty box [ ]. I've also excised all the checked boxes from my review over in https://github.com/openjournals/joss-reviews/issues/3888

General checks

• [x] Contribution and authorship: Has the submitting author (@mcsiple) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

  • Yes, but DESCRIPTION should be updated to be in line with the author list and version number in CITATION.CFF.
  • .zenodo.json has a different author list than the entry at Zenodo: https://zenodo.org/record/4758402 - The repository owner might want to enable auto-archiving in Zenodo for GitHub releases - https://docs.github.com/en/repositories/archiving-a-github-repository/referencing-and-citing-content

Functionality

• [x] Installation: Does installation proceed as outlined in the documentation?

  • warning on install:

    ─ excluding invalid files Subdirectory 'R' contains invalid file names: ‘_disable_autoload.R’

  Could this be added to .Rbuildignore? I'm not sure if it is present to help with running the shiny app as a developer, and would be safe to exclude from the built package for end-users.

• [x] Functionality: Have the functional claims of the software been confirmed?

  • I think arrange needs to be added to the functions to importFrom dplyr on line 15 of 17_plot_proj.R and line 20 of 18_multiplot_proj.R
  • For the model projection plot, it might be useful to use one of the interactive plotting packages like plotly, which would let the user zoom in on certain portions (to better see distinction between trajectories or better identify when extirpation is predicted). In addition, the legend for Bycatch level only has the labels for "{High end / Midpoint / Low end} of bycatch range", and it may be useful to calculate the numbers from the specified bycatch mortality range to make the legend more informative by itself.
  • When toggling between bycatch mortality by "Numbers per year" and "Mortality rate", the interface for selecting Bycatch CV changes between a text box and a slider. Is there a reason for this?
  • The kite plot in the "Advanced Projections" seems to occasionally error out with a "need finite 'ylim' values" message. (e.g. selecting "Sea lion", leaving all parameters at default values, and clicking on "Run projections")
  • The population time series plot seems to fail when both "Show individual projections" and "Show multiple depletion levels" is checked.
  • It'd be ideal to reorder the bycatch level factor so that Low, Midpoint and High are in either increasing or decreasing order.
  • The violin plots are flat for some bycatch levels, resulting in horizontal lines instead of a distribution. There might be a way to indicate such, and perhaps to better show grouping for "bycatch level" within "years after projections start"
  • The design is a bit awkward with the side-by-side of parameters and projection plots. I feel as though I would prefer to have the usage steps proceed from top to bottom, so that I set parameters in a section, then scroll down and click to run projections, then scroll down and see model outputs.
  • You have different kinds of summary outputs for the model projections. It might be nice to add an option to download the raw projection data for someone to do their own calculations.
  • The GAMMS link in the "PBR & PBR calculator" tab doesn't seem to work.
  • It does seem a bit confusing that the input arguments to dynamics() have separate arguments for the life history parameters, whereas in projections(), they're given as a named list. I think this could be made more consistent, and that the list structure is probably better as it is easier to refer to as a single object to pass to multiple functions.

• [x] Functionality: Have the functional claims of the software been confirmed?

  • DESCRIPTION has a duplicated LazyData line that is preventing building the package.
  • There are a few points where you have dplyr::arrange(dplyr::desc(name, sim)) which produces an error. I think it should be something like dplyr::arrange(dplyr::desc(name), dplyr::desc(sim))
  • Is this possible to do? I'm not sure about Shiny limitations.

    For the "Solve for Bycatch" tab, it would be ideal to show all the life history and other model parameters from the Advanced tab here, just so someone using it can confirm that the numbers are correct without having to switch tabs. I also think showing the table of tested bycatch rate values would be good, as it would allow people to then go back to the Advanced tab and test/confirm the results.

Documentation

• [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).

  • Yes, in the readme. The application does contain some functionality to output a report from the "PBR & PBR calculator" tab. I don't know how useful it would be for an end-user to be able to go through model projections in the Shiny app and then output a reproducible R Markdown report, but that could be a consideration as a feature. (not necessary here, but perhaps if stakeholder feedback shows it as a useful inclusion)

• [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?

  • There seems to be some duplication in the "Model description" vignette and the "About the project model" tab of the shiny app. There's some concern about keeping the two synchronized if/when updates happen. Is it possible perhaps to embed one of the Rmarkdown files in the other, so that changes only need to happen in one location?
  • I would like to see some additional description in the "Model description" vignette about the function arguments that the variables map to. The names are generally straightforward, but it would help to have that centralized table of variable names in the vignette.

• [x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?

  • The testing infrastructure is present, but is currently a bit bare-bones.
  • It appears that testing of Shiny applications is still a bit in flux. Shiny is now on ver 1.7.0, but doesn't seem to fully have the testing functions that https://engineering-shiny.org/build-yourself-safety-net.html?q=testing#shinytestserver mentions. One option that I've seen in the rOpenSci community is using VCR to test the interactive components - https://github.com/ropensci/vcr/
  • Regardless of the above, tests could be added for the modeling and calculation functions in the package, possibly along with test datasets to confirm correct computation. The R package book contains details about testing here - https://r-pkgs.org/tests.html

• [x] 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

  • There is some basic info in the package README:
  • The badge link should point to the issues page for this repo.
  • I don't personally find the linked page in R package book useful for guiding new contributors. (and practiced contributors likely already know of the content or won't need it.)
  • Consider a separate CONTRIBUTING.md file that also links to the Code of Conduct, and provides more specific instructions. There is some general info here - https://mozillascience.github.io/working-open-workshop/contributing/

Software paper

• [x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?

  • There is some jargon that is specific to those involved in fisheries management, but the target audience is made clear in the Statement of Need, so I do not feel this is a problem in this section of the paper.

• [x] State of the field: Do the authors describe how this software compares to other commonly-used packages?

  • I found https://github.com/NorskRegnesentral/rSPAMM as another marine mammal assessment package in R. It would be helpful to include a sentence or two on the differences (perhaps the underlying model and intended audience?)

• [x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

  • Some references that were previously unpublished, are now published and the citation info should be updated, e.g. https://www.frontiersin.org/articles/10.3389/fmars.2021.757330/full

[mcsiple]

Updated citations for papers that cite this tool with commit bcc79b09

[mcsiple]

Hi Hao,

I used issue #57 to keep track of all the functionality reviews (thank you for those) and have, I think, addressed all of them. I'm going to address the others as I go, here. I will try to minimize the number of separate comments so you don't get a ton of notifications. If you don't mind, for my own record-keeping and issue tracking purposes, would you mind checking off the sections that you feel are properly addressed? You could do it here or in the JOSS review or both.

General checks

• I have fixed the author list in CITATION.cff
• I would love to turn on auto-archiving for this repo and am going to attempt it once I think I have addressed all of your comments and @tbrown122387 's, to see if it added the other authors. I remember having trouble earlier adding authors to the Zenodo doi.

Functionality

• Fixed disable_autoload issue
• See #57 for revisions to functionality. The only things I didn't incorporate were the order of the legend colors in the kite plot (I spent a couple hours trying to fix the order but have abandoned the attempt; I don't think it affects usability even though it's a little weird) and the side bar layout (we agreed on this layout with the UI folks we consulted, and it is intended to keep the projections plot alongside the input parameters so the users can see the relationship between them)

Documentation

Example usage

• I agree that a Markdown report could be helpful. For now, I am adding this as a feature request and if we get support for it from stakeholders I will add it. (#59 )

Functionality documentation

• I will look into whether I can easily generate the model description vignette from the About the Model tab contents. It shouldn't be too hard but I haven't done it yet.
• I added the code version of the parameters to the table in "About the Model" -- if this is adequate, I will do the same to the other languages and to the vignette.

Automated tests

I still need to work on these. Overlap with @tbrown122387 's comment and definitely something that I need to work on. There are Shiny-specific tests but I think the most important ones are for the embedded functions for population dynamics, etc.

Community guidelines

• Badge link in README has been fixed (now points to Issues, thank you) 8dc3c875
• I think a CONTRIBUTING.md file is a great idea and will work on it, but haven't created it yet. Added as issue #60

Software paper

State of the field

• rSPAMM has been added to the paper -- good find.
• References: updated references in paper.md

[mcsiple]

Hi @ha0ye -- did you get a chance to look at these revisions yet? Normally I would not bug you but I have a meeting coming up where I'm presenting the package and would like to be able to provide them with a status update. Thank you!

[ha0ye]

Yep, sorry. Had a few more functional notes, which I edited into the original post above.

[mcsiple]

OK -- just fixed those two first functionality issues (thanks!). Commit noted above. As for the third bullet point, the parameters do show up in the "Solve for Bycatch" tab when the plot and solution are rendered. The final result looks like this:

That's what you meant for that one, right? LMK if there is anything else and thank you for all the time you've put into testing and reviewing!

[ha0ye]

Hmm, maybe I was thinking of the PBR & PBR calculator tab? Looks good to me, though!

[mcsiple]

Ah, I see what you mean. I added that little table to the PBR tab and put in a translation for it. Thanks! I'm going to close this issue now if that's cool with you!