Comments from JOSS review

[fernandomayer]

Ref.: openjournals/joss-reviews#6878

Hi @meixichen. I'll try to concentrate all my comments here. Please feel free to reply with any queries may appear.

Regarding the package itself

• Installation:

  • I tested both CRAN and remotes::install_github() and everything went fine.
  • When I try to build and check the package locally (with either R CMD and devtools::check(), I have a few notes:
    • In both the build and the check process, the creation of the vignette takes a lot of time. I'm aware this was previously discussed here and I just want to add to it. I often have vignettes that takes lots of time to run, so I adopted this strategy from rOPenSci, and also I use to cache the heavier chunks (and ignore the *_cache directory in both .gitignore and .Rbuildignore). You can see an example here. Also, see this section of R Packages book.
    • When running R CMD build, I get these messages (after a long wait for the vignette to compile)
    • Then running R CMD check --as-cran, I got these messages. Please, see the NOTEs flagged there.

• vignettes:

  • Besides the above, I would suggest:
    • Migrate the helper functions from vignettes/vignette-helper-functions.R to an appropriate file under R/, for example zzz.R or utils.R, even if you don't export those functions. That makes the package more concise, avoids the use of source(), and stays clear in the vignette that those are functions from your package. In this case I would export them as auxiliary functions anyway. See this.
    • This gives the impression that the only option is to use this fixed prior, however this is not true, as the documentation in ?SpatialGEV_fit() states.

      SpatialGEV/vignettes/SpatialGEV-vignette.Rmd

      Line 65 in 3f3af2e

      In this package, a uniform prior $\pi(\tth) \propto 1$ is specified on the fixed effect and hyperparameters

    • Cite the "our paper" here

      SpatialGEV/vignettes/SpatialGEV-vignette.Rmd

      Line 146 in 3f3af2e

      The function `spatialGEV_fit()` in the **SpatialGEV** package is used to find the mode and quadrature of the Laplace approximation to the marginal hyperparameter posterior, which is the basis of a Normal approximation to the posterior of both hyperparameters and random effects referred to as Laplace-MQ in our paper. The function `spatialGEV_sample()` is then used to sample from this distribution via a sparse representation of its precision matrix.

    • There is an extra @ in the last citation here

      SpatialGEV/vignettes/SpatialGEV-vignette.Rmd

      Line 162 in 3f3af2e

      - `method`: A character string "laplace" (default) or "maxsmooth". The former corresponds to the main Laplace method described in @chen-etal21. The latter corresponds to a more computationally efficient but possibly less accurate two-step method known as *Max-and-Smooth* [@hrafnkelsson-etal19]. Details on the Max-and-Smooth approach for fitting a GEV-GP model can be found in @@jhannesson-etal22.

    • The numbers here are incorrect. You should either correct the text (with 50 locations) or the code (with n_test <- 100

      SpatialGEV/vignettes/SpatialGEV-vignette.Rmd

      Line 327 in 3f3af2e

      We randomly sample 100 locations from the simulated dataset `simulatedData` as test locations which are left out. Data from the rest 300 training locations are used for model fitting. We fit a GEV-GP model with random $a$ and $b$ and log-transformed $s$ to the training dataset. The kernel used here is the SPDE approximation to the Matérn covariance function (@lindgren-etal11).

• README.Rmd:

  • Wouldn't it be useful to have a plot of the (marginal) posterior distributions? maybe a generic plot method should be helpful.

Regarding the JOSS paper

• Only one suggestion: a concluding paragraph. The paper is well written and has everything it needs, but it ends very abruptly. A concluding paragraph would be very interesting.

In general it's an excellent software and a well written vignette/paper.