JOSS review: code points

[vankesteren]

Hey @gavinsimpson,

congrats on your submission to JOSS! This is an issue coming out of the JOSS review here. Here is a checklist for points relating to your code / documentation.

• The readme could benefit from a better statement of need in the overview section. Short, to the point, basically "gams can be difficult to interpret, gratia helps with this".
• Why draw() and why not the ggplot2 built-in generic autoplot()? That is for example used in packages like fpp3. I feel like adding an additional generic is maybe overkill? You could have also overloaded plot.gam() (although I do appreciate the issues with that so that may be a bad idea). I also realize that your code is already widely used so that may hinder changing something so fundamental about your API.
• Documentation for the argument "method" in the function appraise() is incomplete. Please document the other methods! (e.g., simulate, which is included in the paper as an example)
• Fix the FIXME statement in the documentation of the data argument in draw.gam().
• The function data_slice() should be better documented, to avoid confusion with another common use of the term "slice" as indexing (e.g., https://stackoverflow.com/questions/509211/how-slicing-in-python-works, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/slice). How are the values for the non-focal covariates computed? are they averages? are they local averages? (they seem to be averages)

I haven't yet had the time to go over the other documentation (it's a lot!), but will try to do so ASAP.

[gavinsimpson]

@vankesteren Thanks for the comments/review

[gavinsimpson]

Why draw() and why not the ggplot2 built-in generic autoplot()? That is for example used in packages like fpp3. I feel like adding an additional generic is maybe overkill? You could have also overloaded plot.gam() (although I do appreciate the issues with that so that may be a bad idea). I also realize that your code is already widely used so that may hinder changing something so fundamental about your API.

I thought about this a lot having used autoplot() for a unreleased (i.e. not-on-cran) package ggvegan that extends ggplot graphics to vegan's plots. I really don't like the name autoplot(), not least that it is 4 characters too many to type; not being able to extend ggplot() was, IMHO, a mistake in the implementation but I understand the reasons why; plot() is for base R graphics and I think it is a mistake to use that generic for ggplot-based graphics.

I appreciate that draw is an new generic, but it's pretty clearly signposted and used consistently across the package, including in the intro materials.

So this one I'm not going to fix/change.

[gavinsimpson]

@vankesteren I have addressed each of the remaining points. If the changes do not go far enough, reopen this and I can take another stab.