search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
721 stars 38 forks source link

[REVIEW]: fishStan: Hierarchical Bayesian models for fisheries #3444

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: !--author-handle-->@rerickson-usgs<!--end-author-handle-- (Richard Erickson) Repository: https://code.usgs.gov/umesc/quant-ecology/fishstan/ Branch with paper.md (empty if default branch): Version: v2.0.0 Editor: !--editor-->@marcosvital<!--end-editor-- Reviewers: @MikeKaller, @Cole-Monnahan-NOAA Archive: 10.5066/P9TT3ILO

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/34bd0a9673510f204ee01fe1536b2196"><img src="https://joss.theoj.org/papers/34bd0a9673510f204ee01fe1536b2196/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/34bd0a9673510f204ee01fe1536b2196/status.svg)](https://joss.theoj.org/papers/34bd0a9673510f204ee01fe1536b2196)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@BrandonEdwards & @MikeKaller, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @marcosvital know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @MikeKaller

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @Cole-Monnahan-NOAA

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @BrandonEdwards , @MikeKaller it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.09 s (662.5 files/s, 54412.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                               44            282           1057           1637
Rmd                              6            217            766            380
Markdown                         6             86              0            343
TeX                              1              6              0             54
JSON                             1              7              0             41
YAML                             1             10              0             41
C/C++ Header                     1              0              1              0
-------------------------------------------------------------------------------
SUM:                            60            608           1824           2496
-------------------------------------------------------------------------------

Statistical information for the repository '39c69dfe28f0b28ed89bba83' was
gathered on 2021/07/02.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Richard A Erickson               2            39             38          100.00

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Richard A Erickson            1            2.6          0.0              100.00
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1002/nafm.10515 is OK

MISSING DOIs

- 10.1016/j.fishres.2015.03.009 may be a valid DOI for title: Spatial and temporal variability in growth of Southern Flounder (Paralichthys lethostigma)
- 10.1201/9781315371986 may be a valid DOI for title: Introductory fisheries analyses with R

INVALID DOIs

- None
whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

marcosvital commented 3 years ago

Dear @rerickson-usgs: your manuscript will be reviewed in this issue, and you can reply any comments and suggestions that the reviewers might address right here.

marcosvital commented 3 years ago

@BrandonEdwards and @MikeKaller: thank you again for for accepting review this submission for JOSS.

Even if you are not starting the review right now, please accept the invite, as it has an expiration date (there is a link under Reviewer instructions & questions and you should also get an email notification). Furthermore, please check the instructions and checklists above, and let me know if you need any assistance.

You can also tag @rerickson-usgs if you need to ask specific questions about the submission or to address any changes that might be necessary in the submitted paper or in the repository.

Finally, please let me know if you need any assistance while reviewing.

whedon commented 3 years ago

:wave: @MikeKaller, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @BrandonEdwards , please update us on how your review is going (this is an automated reminder).

raerickson commented 3 years ago

@marcosvital Hope all is well with you. Any updates?

marcosvital commented 3 years ago

Hi, @rerickson-usgs, I was about to ask the reviewers for an update. So here we go:

@BrandonEdwards and @MikeKaller, any news on this review? Let me know if you need any assistance. And if you haven't started yet, I might need to send a new invitation to allow you to check on the boxes as you move on.

marcosvital commented 3 years ago

Hello again, @rerickson-usgs! It seems that @MikeKaller is generally satisfied with the submission, but there are three issues linked on his reviewing checklist (check above). Did you had the chance to look into them? When addressing them, please create a copy on the original repository if @MikeKaller alredy didn't do that.

marcosvital commented 3 years ago

Dear @BrandonEdwards, did you had the chance to start reviewing this submission? Let me know if you need any assistance, ok?

raerickson commented 3 years ago

@marcosvital Thank you for following up. I will address @MikeKaller's comments. They make sense.

Also, for your information, once I get the review back from the 2nd reviewer, I will need to complete the USGS Review process before returning the final submission to the journal.

marcosvital commented 3 years ago

Thank you for your feedback, @raerickson.

@BrandonEdwards, did you had the chance to start reviewing? Please let us know, ok?

raerickson commented 2 years ago

@marcosvital any updates? Also, would seeing the USGS software review help you?

marcosvital commented 2 years ago

Hi, @raerickson, I'm sorry this is taking so long.

Please, do share with us the USGS software review.

I will try to find an additional reviewer for your submission as fast as possible, so we can finish this.

marcosvital commented 2 years ago

Dear @Cole-Monnahan-NOAA

You answered before about the possibility to review this submission to JOSS. At that time, we had two reviewers, so we started the process with them. Unfortunately one of them could not continue to review, so I would like to know if you are still available.

raerickson commented 2 years ago

@marcosvital I only have access to one of the two reviews. (They go to a dark archive I do not have access to and I got a new computer this summer).

Here is one of the reviews: 201109 fishStan v2 review - APA.pdf. The reviewer also reviewed an earlier version of the package, where she had more comments (and taught me about linting!).

Also, I have long since deleted the reconciliation branch and my response was also lost for the reasons listed above.

Cole-Monnahan-NOAA commented 2 years ago

@marcosvital yeah I'm still available... please advise on how I should proceed.

MikeKaller commented 2 years ago

@macrosvital my review was submitted months ago. Is there anything else that I needed to do?

From: Cole Monnahan @.> Sent: Monday, November 29, 2021 6:38 PM To: openjournals/joss-reviews @.> Cc: Michael D Kaller @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: fishStan: Hierarchical Bayesian models for fisheries (#3444)

@marcosvitalhttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmarcosvital&data=04%7C01%7Cmkalle1%40lsu.edu%7C444a591d5b8441f3b67a08d9b399b5bb%7C2d4dad3f50ae47d983a09ae2b1f466f8%7C0%7C0%7C637738295025092336%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=sP00y%2BTDNdX7eZzZh5tJP1kZwwvgQfo532nWoXptCfc%3D&reserved=0 yeah I'm still available... please advise on how I should proceed.

- You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenjournals%2Fjoss-reviews%2Fissues%2F3444%23issuecomment-982168556&data=04%7C01%7Cmkalle1%40lsu.edu%7C444a591d5b8441f3b67a08d9b399b5bb%7C2d4dad3f50ae47d983a09ae2b1f466f8%7C0%7C0%7C637738295025102297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=GJhrkqgCPeqwI5KdSVnGYPwTkarbtu3FtewZbf%2FNQi8%3D&reserved=0, or unsubscribehttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAUTKUYTOH6SPIGUGV3XR4ALUOQMHZANCNFSM47WRBAYQ&data=04%7C01%7Cmkalle1%40lsu.edu%7C444a591d5b8441f3b67a08d9b399b5bb%7C2d4dad3f50ae47d983a09ae2b1f466f8%7C0%7C0%7C637738295025102297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=UX11lE%2Fxc9Uk%2B6M0JPMMasMn3%2FyjAOm35sltGDcFLzY%3D&reserved=0. Triage notifications on the go with GitHub Mobile for iOShttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7Cmkalle1%40lsu.edu%7C444a591d5b8441f3b67a08d9b399b5bb%7C2d4dad3f50ae47d983a09ae2b1f466f8%7C0%7C0%7C637738295025102297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=rryR%2Fq1sRd4q8HaE3fPd8RDxZzr4U54XnWKF7Iqf8SQ%3D&reserved=0 or Androidhttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub&data=04%7C01%7Cmkalle1%40lsu.edu%7C444a591d5b8441f3b67a08d9b399b5bb%7C2d4dad3f50ae47d983a09ae2b1f466f8%7C0%7C0%7C637738295025112250%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=bYuLEfKdKuHvCX4QyNSRtiheqB2UY5Q%2F4q7%2FMbiX1og%3D&reserved=0.

marcosvital commented 2 years ago

@marcosvital yeah I'm still available... please advise on how I should proceed.

Thank you, @Cole-Monnahan-NOAA. I'll add you as a reviewer and guide you through the process.

marcosvital commented 2 years ago

Dear @MikeKaller, thank you for all your work on this submission. Since you have finished your review, there is no need for you to follow this issue anymore, so feel free to turn the notifications off.

marcosvital commented 2 years ago

@whedon remove @BrandonEdwards as reviewer

whedon commented 2 years ago

OK, @BrandonEdwards is no longer a reviewer

marcosvital commented 2 years ago

@whedon add @Cole-Monnahan-NOAA as reviewer

whedon commented 2 years ago

OK, @Cole-Monnahan-NOAA is now a reviewer

marcosvital commented 2 years ago

@Cole-Monnahan-NOAA, I just added a review checklist for you on the top of this issue. Please check here if you received the invitation to work on this issue, and after that you should be able to check the boxes from the list.

Please check the instructions and the checklist, and let me know if you need any assistance. You can also tag @rerickson-usgs if you need to ask specific questions about the submission or to address any changes that might be necessary in the submitted paper or in the repository.

Since we already have one complete review on this issue and @rerickson-usgs provided us with the USGS software review, you can check on both if you feel it could be helpful for you.

Let me know if you have any questions or face any difficulties while reviewing, ok?

Cole-Monnahan-NOAA commented 2 years ago

@marcosvital I'm working through this review today. Where do I submit my comments beyond the checklist? Just drop them in here? I didn't see the other reviewer comments.

marcosvital commented 2 years ago

@marcosvital I'm working through this review today. Where do I submit my comments beyond the checklist? Just drop them in here? I didn't see the other reviewer comments.

Thank you, @Cole-Monnahan-NOAA You can submit your comments here in this very Github issue, or create a new issues if you think it's better (for example, if you find something that could need more discussion). You can create new issues by using the 'convert to issue' function that will show up at the right of the checkbox entry.

Cole-Monnahan-NOAA commented 2 years ago

I find it easier to just paste in a normal review here. Hopefully that is sufficient. Thanks -Cole

The ‘fishStan’ R package and accompanying paper are meant for fisheries professionals fitting primarily hierarchical growth models (length at age). The paper is very short (<2 pages) and has few details. Furthermore, the R package was difficult (and very slow) to install meaning the examples and vignettes were not available easily. I think this is a sufficient package to share with limited close colleagues, but it does not appear to be ready for wider usage. I'm strongly supportive of providing transparent, open-source tools for working professionals and fishStan attempts to do that. There's a wide gradient of effort that can be put into a package, and I cannot judge whether this one is sufficient for publication in JOSS. I leave that up to the editors.

I am not really the target audience for such a package as I have have expertise developing similar Stan models. But still, installation should be quick and seamless. Two immediate thoughts to remedy this: (1) provide a pkgdown site so users can browse the vignettes without installing the package to help gauge its usefulness to their particular needs, (2) instead of running examples in the vignette (requires compilation and long estimation time) you could package a fitted real model and have the diagnostics/outputs be created dynamically.

Manuscript My general thought is that the manuscript does not add much over the vignettes and seems very short.

L15: “a fisheries managers”

L26: I disagree that there are few ecological models written in Stan. There are whole books on them (including the famous Kery and Schaub 2011 book, see https://mc-stan.org/users/documentation/external and https://stanecology.github.io/resources_and_books.html). In general the package does not reference the myriad hierarchical growth and maturity studies in the literature.

R package First, I had issues installing the package (see installation issues below for details). I had to clone and do a local install and still could not get the vignettes to work right. As the vignette contains key examples I found it difficult to get going quickly with provided examples. Those issues could be machine-specific but if not I suggest fixing them to smooth out installation. I also ran the tests locally and they failed (also below).

As a Stan user I would like to see more integration with standard Bayesian tools. Specifically using ShinyStan to explore model convergence/properties (please liberally link to the Stan documentation on these topics), posterior fits with data, posterior predictive distributions vs observed data, and model selection with, ideally, PSIS-LOO (‘loo’). These are widely seen as key components in a Bayesian analysis and should not be too hard to implement in provided examples so that users can adapt code to their own situations. With respect to convergence diagnostics I suggest adding some examples as well. Given these models estimate both observation and process errors, it would also be good for a user to easily compare those in a meaningful way.

In my experience with growth models, the likelihood has been assumed to be lognormal. I think this makes more sense than the normal error structure assumed by the authors. Why was that choice made?

The function documentation is pretty minimal and would be worth expanding. There are also spelling errors (“prios for gamma mean”). I suggest the authors take time to improve the help files.

Stan models

I had a quick look through the Stan models and it appears they are well organized and structured (functions, using generated quantities sections, etc.), and written with efficiency in mind (vectorization, non-centering, etc.).

Why is the following parameter bounded above by pi/2? Why not just bound tau and use directly as a parameter? 

vector<lower=0,upper=pi()/2>[n_ind_predictors] tau_unif;
  for (k in 1:n_ind_predictors) tau[k] = 2.5 * tan(tau_unif[k]);

You have an implicit uniform prior here, what does that look like on the tau scale? If I simulate (prior pushforward) I get some really crazy values and they’re not even positive. What is going on here?

> summary(2.5*tan(rnorm(1e5,0,pi/2)))
     Min.   1st Qu.    Median      Mean   3rd Qu.      Max. 
 -43393.1      -2.4       0.0      16.0       2.4 1203913.6

Installation issues I was able to remotely install the R package. But when I tried to also build vignettes it failed with the following error (truncated):

Warning: The vignette title specified in \VignetteIndexEntry{} is different from the title in the YAML metadata. The former is "Example with real dat", and the latter is "Example with real data". If that is intentional, you may set options(rmarkdown.html_vignette.check_title = FALSE) to suppress this check.
   --- finished re-building 'real_data.Rmd'

   --- re-building 'vonBertalanffy.Rmd' using rmarkdown
   Quitting from lines 18-21 (vonBertalanffy.Rmd) 
   Error: processing vignette 'vonBertalanffy.Rmd' failed with diagnostics:
   there is no package called 'ggthemes'
   --- failed re-building 'vonBertalanffy.Rmd'

I tried installing ggthemes, cloning the repo locally and installing directly using devtools and I still had issues. It took quite a long time to build the vignettes (> 1 hour) and they still would not load in RStudio.

I also ran the automated tests (testthat) and they failed as follows

------------------------------------------------------------------------------------------------------------------------
v |        11 | test function to create projection inputs                                                               
v |         2 | Gompertz functions test                                                                                 
v |         3 | test Gompertz stan functions [113.8s]                                                                   
v |         2 | Galluci Quinn functions test                                                                            
v |        78 | test binomial code and extract, rename functions [369.1s]                                               
v |         3 | test gq function [38.3s]                                                                                
/ |         0 | helper functions test                                                                                   Error in x$.self$finalize() : attempt to apply non-function
In addition: Warning message:
package 'testthat' was built under R version 4.0.5 
Error in (function (x)  : attempt to apply non-function
v |         4 | helper functions test [26.5s]                                                                           
v |         3 | test ling functions [46.3s]                                                                             
- |         1 | test linear model                                                                                       
==== C stack trace ===============================

    (No symbol) [0x0x63d6f540]
    R_RunWeakRefFinalizer [0x0x6c83326f+383]
    R_RunWeakRefFinalizer [0x0x6c833507+1047]
    Rf_eval [0x0x6c7fb81a+330]
    Rf_eval [0x0x6c7fc0f9+2601]
    Rf_eval [0x0x6c7fc578+3752]
    R_initAssignSymbols [0x0x6c7f1321+51121]
    Rf_eval [0x0x6c7fb841+369]
    R_cmpfun1 [0x0x6c7fd547+1191]
    Rf_applyClosure [0x0x6c7fe7aa+442]
    R_initAssignSymbols [0x0x6c7f2994+56868]
    Rf_eval [0x0x6c7fb841+369]
    R_cmpfun1 [0x0x6c7fd547+1191]
    Rf_applyClosure [0x0x6c7fe7aa+442]
    R_initAssignSymbols [0x0x6c7f2994+56868]
    Rf_eval [0x0x6c7fb841+369]
    R_cmpfun1 [0x0x6c7fd547+1191]
    Rf_applyClosure [0x0x6c7fe7aa+442]
    R_forceAndCall [0x0x6c801002+338]
    R_forceAndCall [0x0x6c8013dc+1324]
    R_initAssignSymbols [0x0x6c7ebe3b+29387]
    Rf_eval [0x0x6c7fb841+369]
    R_cmpfun1 [0x0x6c7fd547+1191]
    Rf_applyClosure [0x0x6c7fe7aa+442]
    R_initAssignSymbols [0x0x6c7f2994+56868]
    Rf_eval [0x0x6c7fb841+369]
    R_cmpfun1 [0x0x6c7fd547+1191]
    Rf_applyClosure [0x0x6c7fe7aa+442]
    R_initAssignSymbols [0x0x6c7f2994+56868]
    Rf_eval [0x0x6c7fb841+369]
    R_cmpfun1 [0x0x6c7fd547+1191]
    Rf_applyClosure [0x0x6c7fe7aa+442]
    R_initAssignSymbols [0x0x6c7f2994+56868]
    Rf_eval [0x0x6c7fb841+369]
    R_cmpfun1 [0x0x6c7fd547+1191]
    R_execMethod [0x0x6c7febfe+526]
    R_dispatchGeneric [0x0x64a42e22+1250]
    R_set_standardGeneric_ptr [0x0x6c83cd58+1128]
    R_initAssignSymbols [0x0x6c7ec15e+30190]
    Rf_eval [0x0x6c7fb841+369]
    R_cmpfun1 [0x0x6c7fd547+1191]
    Rf_applyClosure [0x0x6c7fe7aa+442]
    R_initAssignSymbols [0x0x6c7f2994+56868]
    Rf_eval [0x0x6c7fb841+369]
    R_cmpfun1 [0x0x6c7fd547+1191]
    Rf_applyClosure [0x0x6c7fe7aa+442]
    Rf_eval [0x0x6c7fb9ec+796]
    Rf_eval [0x0x6c7fc0f9+2601]
    Rf_eval [0x0x6c7fbb44+1140]
    Rf_eval [0x0x6c7fc0f9+2601]
    Rf_eval [0x0x6c7fc578+3752]
    R_initAssignSymbols [0x0x6c7f1321+51121]
    Rf_eval [0x0x6c7fb841+369]
    Rf_eval [0x0x6c7fc0f9+2601]
    Rf_eval [0x0x6c7fc578+3752]
    R_initAssignSymbols [0x0x6c7f1321+51121]
    Rf_eval [0x0x6c7fb841+369]
    R_cmpfun1 [0x0x6c7fd547+1191]
    Rf_applyClosure [0x0x6c7fe7aa+442]
    R_initAssignSymbols [0x0x6c7f2994+56868]
    Rf_eval [0x0x6c7fb841+369]
    R_cmpfun1 [0x0x6c7fd547+1191]

Exited with status -1073741819.
raerickson commented 2 years ago

@marcosvital Thank you for working with me through this long process. I have approval from my agency to publish. I have addressed both reviewers' comments as branches' on the software's page: https://code.usgs.gov/umesc/quant-ecology/fishstan Please let me know what are the next steps.

raerickson commented 2 years ago

@marcosvital What' the next steps? I've addressed the reviewers' comments and gotten USGS approval.

marcosvital commented 2 years ago

Hi, @raerickson! Sorry about the delay. It seems we are almost finished here.

@Cole-Monnahan-NOAA, are you satisfied with the current version? If you are, we need you to finish the checklist, so we can move on.

Cole-Monnahan-NOAA commented 2 years ago

I'm looking at this branch https://code.usgs.gov/umesc/quant-ecology/fishstan/-/commits/joss-cole

and don't see much in the way of addressing my comments. Am I missing something? Could you point me to the commits addressing the issues above? thanks!

raerickson commented 2 years ago

@Cole-Monnahan-NOAA I'll include my reconciliation here. Sorry for the frustration this system is causing you.

Thank you for your review, @Cole-Monnahan-NOAA

I find it easier to just paste in a normal review here. Hopefully that is sufficient. Thanks -Cole

The fishStanâ R package and accompanying paper are meant for fisheries professionals fitting primarily hierarchical growth models (length at age). The paper is very short (\<2 pages) and has few details. Furthermore, the R package was difficult (and very slow) to install meaning the examples and vignettes were not available easily. I think this is a sufficient package to share with limited close colleagues, but it does not appear to be ready for wider usage. I’m strongly supportive of providing transparent, open-source tools for working professionals and fishStan attempts to do that. There’s a wide gradient of effort that can be put into a package, and I cannot judge whether this one is sufficient for publication in JOSS. I leave that up to the editors.

JOSS's example paper is two pages long as well, https://joss.theoj.org/papers/10.21105/joss.00388. We are open to pull requests to speed up our package's build time.

I am not really the target audience for such a package as I have have expertise developing similar Stan models. But still, installation should be quick and seamless. Two immediate thoughts to remedy this:

  1. provide a pkgdown site so users can browse the vignettes without installing the package to help gauge its usefulness to their particular needs,

We will investigate pkgdown for future packages. We do not think we are able to use that yet, but might be once USGS and DOI are officially part of GitHub, hopefully early next year.

Manuscript My general thought is that the manuscript does not add much over the vignettes and seems very short.

Correct, JOSS is for short papers, see the section, What should my paper contain?.

L15: 'a fisheries managers'

Fixed.

L26: I disagree that there are few ecological models written in Stan. There are whole books on them (including the famous Kery and Schaub 2011 book, see https://mc-stan.org/users/documentation/external and https://stanecology.github.io/resources_and_books.html). In general the package does not reference the myriad hierarchical growth and maturity studies in the literature.

We removed this line. If there are any specific references you would like us to cite, please indicate them and we would be happy to consider them. Or, we would also welcome a pull request.

R package First, I had issues installing the package (see installation issues below for details). I had to clone and do a local install and still could not get the vignettes to work right. As the vignette contains key examples I found it difficult to get going quickly with provided examples. Those issues could be machine-specific but if not I suggest fixing them to smooth out installation. I also ran the tests locally and they failed (also below).

We were able to install the computer on @danStich's Window's computer and USGS's cloud-hosted RStudio, but not on @raerickson's USGS Window's machine.

The tests passed on our machines. We have reached out to your NOAA's email address to off help troubleshooting the install. We have had trouble ourselves with RStan and government computers.

# Testing fishStan
# | F W S  OK | Context
# |         4 | catch_curve_stan test [7.4s]                                                     
# |         2 | Test catch_curve                                                                 
# |        11 | test function to create projection inputs                                        
# |         3 | test Gompertz stan functions [100.4s]                                            
# |         2 | Gompertz functions test                                                          
# |         2 | Galluci Quinn functions test                                                     
# |        78 | test binomial code and extract, rename functions [129.3s]                        
# |         3 | test gq function [44.3s]                                                         
# |         4 | helper functions test [45.2s]                                                    
# |         3 | test ling functions [53.0s]                                                      
# |        13 | test linear model [169.9s]                                                       
# |         4 | Gompertz simulation function test                                                
# |         2 | Galluci Quinn functions test                                                     
# |         2 | test VB without t0 functions [14.6s]                                             
# |         3 | test VB functions [24.9s]                                                        
# |         3 | von B t0 functions test using t0 = 0                                             
# |         3 | von B functions test                                                             

...
Duration: 589.2 s

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 142 ]

As a Stan user I would like to see more integration with standard Bayesian tools. Specifically using ShinyStan to explore model convergence/properties (please liberally link to the Stan documentation on these topics), posterior fits with data, posterior predictive distributions vs observed data, and model selection with, ideally, PSIS-LOO (‘loo'). These are widely seen as key components in a Bayesian analysis and should not be too hard to implement in provided examples so that users can adapt code to their own situations. With respect to convergence diagnostics I suggest adding some examples as well. Given these models estimate both observation and process errors, it would also be good for a user to easily compare those in a meaningful way.

We welcome a pull request for this. We have added in text about this into the README.md file:

We expect users of `fishStan` to be familiar with Bayesian statistics.
A understanding of the subject area will also help the users to apply the models to their area (e.g., fisheries biology).
Furthermore, the user will want use tools to ensure their models have converged such as [`traceplot()`](https://mc-stan.org/rstan/reference/stanfit-method-traceplot.html) or looking at the model diagnostics.
Likewise, tools such as 
[`shinystan`](https://mc-stan.org/users/interfaces/shinystan) or [`loo`](https://mc-stan.org/users/interfaces/loo) may also be used with model results.

In my experience with growth models, the likelihood has been assumed to be lognormal. I think this makes more sense than the normal error structure assumed by the authors. Why was that choice made?

We were following Midway et al. (2015). We transformed the coefficients to the log-scale in growth_models.stan, for example:

row_vector make_von_b_placeholder(
  int n_sites,
  matrix mu_beta_cor,
  vector mu_gamma,
  int n_obs,
  int[] jj,
  row_vector age
  ){

  row_vector[n_sites]  l_inf = //theoretical maximum length
    exp(mu_beta_cor[1] + mu_gamma[1]);
  row_vector[n_sites]  k_growth = // growth coefficient
    exp(mu_beta_cor[2]  + mu_gamma[2]);
  row_vector[n_sites]  t0 = // hypothetical age at which fish's size = 0
    exp(mu_beta_cor[3] + mu_gamma[3]) - 10.0;

  row_vector[n_obs] von_b_placeholder =
    l_inf[jj]  .*  (1.0 - exp( - k_growth[jj] .* (age - t0[jj])));

  return von_b_placeholder;
  }

The function documentation is pretty minimal and would be worth expanding. There are also spelling errors ('prios for gamma mean'). I suggest the authors take time to improve the help files.

We spell checked all of the documentation files and re-ran devtools::document()

To address the other reviewer's comments, we added a vignette using real data. We are unsure which branch this reviewer was reviewing.

Stan models

I had a quick look through the Stan models and it appears they are well organized and structured (functions, using generated quantities sections, etc.), and written with efficiency in mind (vectorization, non-centering, etc.).

Why is the following parameter bounded above by pi/2? Why not just bound tau\ and use directly as a parameter?

vector<lower=0,upper=pi()/2>[n_ind_predictors] tau_unif;
  for (k in 1:n_ind_predictors) tau[k] = 2.5 * tan(tau_unif[k]);

You have an implicit uniform prior here, what does that look like on the tau scale? If I simulate (prior pushforward) I get some really crazy values and they're not even positive. What is going on here?

> summary(2.5*tan(rnorm(1e5,0,pi/2)))
     Min.   1st Qu.    Median      Mean   3rd Qu.      Max. 
 -43393.1      -2.4       0.0      16.0       2.4 1203913.6

We followed Section 1.13 of the Stan User manual.

We also think your simulation in R is incorrect. Here is a simulation that that yields non-negative values:

library(truncnorm)
tau_unif <- ((rtruncnorm(1e5, a=0, b=pi/2, mean = 0, sd = 1)))

tau <- 2.5 * tan(tau_unif)

summary(tau)
    Min.  1st Qu.   Median     Mean  3rd Qu.     Max. 
     0.0      0.7      1.7     14.2      3.6 336663.0

Installation issues I was able to remotely install the R package. But when I tried to also build vignettes it failed with the following error (truncated):

Please see our prevoius comments about the Install. We also emailed the reviewer at NOAA to offer to help.

Warning: The vignette title specified in \VignetteIndexEntry{} is different from the title in the YAML metadata. The former is "Example with real dat", and the latter is "Example with real data". If that is intentional, you may set options(rmarkdown.html_vignette.check_title = FALSE) to suppress this check.
   --- finished re-building 'real_data.Rmd'

   --- re-building 'vonBertalanffy.Rmd' using rmarkdown
   Quitting from lines 18-21 (vonBertalanffy.Rmd) 
   Error: processing vignette 'vonBertalanffy.Rmd' failed with diagnostics:
   there is no package called 'ggthemes'
   --- failed re-building 'vonBertalanffy.Rmd'

I tried installing ggthemes, cloning the repo locally and installing directly using devtools and I still had issues. It took quite a long time to build the vignettes (> 1 hour) and they still would not load in RStudio.

I also ran the automated tests (testthat) and they failed as follows

------------------------------------------------------------------------------------------------------------------------
v |        11 | test function to create projection inputs                                                               
v |         2 | Gompertz functions test                                                                                 
v |         3 | test Gompertz stan functions [113.8s]                                                                   
v |         2 | Galluci Quinn functions test                                                                            
v |        78 | test binomial code and extract, rename functions [369.1s]                                               
v |         3 | test gq function [38.3s]                                                                                
/ |         0 | helper functions test                                                                                   Error in x$.self$finalize() : attempt to apply non-function
In addition: Warning message:
package 'testthat' was built under R version 4.0.5 
Error in (function (x)  : attempt to apply non-function
v |         4 | helper functions test [26.5s]                                                                           
v |         3 | test ling functions [46.3s]                                                                             
- |         1 | test linear model                                                                                       
==== C stack trace ===============================

  (No symbol) [0x0x63d6f540]
  R_RunWeakRefFinalizer [0x0x6c83326f+383]
  R_RunWeakRefFinalizer [0x0x6c833507+1047]
  Rf_eval [0x0x6c7fb81a+330]
  Rf_eval [0x0x6c7fc0f9+2601]
  Rf_eval [0x0x6c7fc578+3752]
  R_initAssignSymbols [0x0x6c7f1321+51121]
  Rf_eval [0x0x6c7fb841+369]
  R_cmpfun1 [0x0x6c7fd547+1191]
  Rf_applyClosure [0x0x6c7fe7aa+442]
  R_initAssignSymbols [0x0x6c7f2994+56868]
  Rf_eval [0x0x6c7fb841+369]
  R_cmpfun1 [0x0x6c7fd547+1191]
  Rf_applyClosure [0x0x6c7fe7aa+442]
  R_initAssignSymbols [0x0x6c7f2994+56868]
  Rf_eval [0x0x6c7fb841+369]
  R_cmpfun1 [0x0x6c7fd547+1191]
  Rf_applyClosure [0x0x6c7fe7aa+442]
  R_forceAndCall [0x0x6c801002+338]
  R_forceAndCall [0x0x6c8013dc+1324]
  R_initAssignSymbols [0x0x6c7ebe3b+29387]
  Rf_eval [0x0x6c7fb841+369]
  R_cmpfun1 [0x0x6c7fd547+1191]
  Rf_applyClosure [0x0x6c7fe7aa+442]
  R_initAssignSymbols [0x0x6c7f2994+56868]
  Rf_eval [0x0x6c7fb841+369]
  R_cmpfun1 [0x0x6c7fd547+1191]
  Rf_applyClosure [0x0x6c7fe7aa+442]
  R_initAssignSymbols [0x0x6c7f2994+56868]
  Rf_eval [0x0x6c7fb841+369]
  R_cmpfun1 [0x0x6c7fd547+1191]
  Rf_applyClosure [0x0x6c7fe7aa+442]
  R_initAssignSymbols [0x0x6c7f2994+56868]
  Rf_eval [0x0x6c7fb841+369]
  R_cmpfun1 [0x0x6c7fd547+1191]
  R_execMethod [0x0x6c7febfe+526]
  R_dispatchGeneric [0x0x64a42e22+1250]
  R_set_standardGeneric_ptr [0x0x6c83cd58+1128]
  R_initAssignSymbols [0x0x6c7ec15e+30190]
  Rf_eval [0x0x6c7fb841+369]
  R_cmpfun1 [0x0x6c7fd547+1191]
  Rf_applyClosure [0x0x6c7fe7aa+442]
  R_initAssignSymbols [0x0x6c7f2994+56868]
  Rf_eval [0x0x6c7fb841+369]
  R_cmpfun1 [0x0x6c7fd547+1191]
  Rf_applyClosure [0x0x6c7fe7aa+442]
  Rf_eval [0x0x6c7fb9ec+796]
  Rf_eval [0x0x6c7fc0f9+2601]
  Rf_eval [0x0x6c7fbb44+1140]
  Rf_eval [0x0x6c7fc0f9+2601]
  Rf_eval [0x0x6c7fc578+3752]
  R_initAssignSymbols [0x0x6c7f1321+51121]
  Rf_eval [0x0x6c7fb841+369]
  Rf_eval [0x0x6c7fc0f9+2601]
  Rf_eval [0x0x6c7fc578+3752]
  R_initAssignSymbols [0x0x6c7f1321+51121]
  Rf_eval [0x0x6c7fb841+369]
  R_cmpfun1 [0x0x6c7fd547+1191]
  Rf_applyClosure [0x0x6c7fe7aa+442]
  R_initAssignSymbols [0x0x6c7f2994+56868]
  Rf_eval [0x0x6c7fb841+369]
  R_cmpfun1 [0x0x6c7fd547+1191]

Exited with status -1073741819.
Cole-Monnahan-NOAA commented 2 years ago

@raerickson thanks for pasting that in and responding to my thoughts

@marcosvital I support this MS moving forward and have completed the checklist. Let me know if there is more to be done on my end. Thanks -Cole

raerickson commented 2 years ago

@marcosvital bump

marcosvital commented 2 years ago

Alright! Thank you, @Cole-Monnahan-NOAA and @MikeKaller for all the effort you put in reviewing this submission.

marcosvital commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1002/nafm.10515 is OK

MISSING DOIs

- 10.1016/j.fishres.2015.03.009 may be a valid DOI for title: Spatial and temporal variability in growth of Southern Flounder (Paralichthys lethostigma)
- 10.1201/9781315371986 may be a valid DOI for title: Introductory fisheries analyses with R

INVALID DOIs

- https://doi.org/10.5066/P9IAOZ8G is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.3996/JFWM-20-070 is INVALID because of 'https://doi.org/' prefix
- https://doi.org/10.5066/P9Q6SUML is INVALID because of 'https://doi.org/' prefix
marcosvital commented 2 years ago

@raerickson, can you check those potentially missing DOIs and correct the invalid? Once you've done that, we will proceed to have a final look on the manuscript to see if everything is ok.

raerickson commented 2 years ago

@marcosvital I have checked the DOIs and updated them in the master branch.

Also, is it okay if I delete the JOSS branch and reviewer branches?

marcosvital commented 2 years ago

@editorialbot check references

editorialbot commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.fishres.2015.03.009 is OK
- 10.1201/9781315371986 is OK
- 10.1002/nafm.10515 is OK
- 10.1002/nafm.10515 is OK
- 10.5066/P9IAOZ8G is OK
- 10.3996/JFWM-20-070 is OK
- 10.5066/P9Q6SUML is OK

MISSING DOIs

- None

INVALID DOIs

- None
marcosvital commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

marcosvital commented 2 years ago

@raerickson yes, it's ok to delete those branches.

And please take a careful look at the new pdf version of the manuscript above. Let me know if everything is fine or if you need to make any final corrections.

raerickson commented 2 years ago

@marcosvital I fixed one small typo and updated the draft in my manuscript.

On line 34: I changed "liner" to be "linear"

Please let me know if you need anything else.

marcosvital commented 2 years ago

Thank you, @raerickson! I'll take a look at the manuscript, and will let you know if there is anything else to be done.

marcosvital commented 2 years ago

@editorialbot generate pdf