Various minor roxygen/package structure/vignette suggestions.

[paciorek]

@kenkellner I've looked through the vignette, Roxygen, and package structure files and made various (mostly nit-picky) suggestions in this PR. I don't think anything will be controversial, but feel free to modify those you don't like.

Other things of note:

• In the LINPRED roxygen, I am thinking the centering argument could be better explained.
• You'll need roxygen info for the modelInfo and .env args for some of the functions in LINPRED.R. I'm sure R CMD check will catch any other such cases.
• Some of the abbreviations in variable naming feel a bit awkward, in particular modMatNames. I'd vote for modelMatrixNames despite its length. But then I also wrote a >200 page thesis, so you might take that with a grain of salt. Perhaps @danielturek can vote my suggestion up or down.
• The LKJ default eta value should be 1 to get uniformity over correlation matrices. The 1.3 in the manual is arbitrary, and this now has me wondering if we should change that...
• I'm pondering whether we should use the name priors rather than priorSpecs.
• In the vignette in the LINPRED_PRIORS section, I think we could make more clear that the user would often not need to call LINPRED_PRIORS directly. The LINPRED section is prefaced with a comment along these lines that could be repeated for the LINPRED_PRIORS section.
• The formulaHandler functions all have basically the same roxygen and these are not user-facing anyway. I suggest a single roxygen block with the use of @alias so that all of them refer to the same roxygen.
• Actually, beyond that, if these are not user-facing do they need to be exported, and if not, then we might not need roxygen anyway.

I have on my todo list to take a bit more time and look at the tests and try to think of corner cases. I hope to do that by end of next week.

As far as code review, I suspect I will not have time to take a thorough look.

[kenkellner]

Thanks for the review! A few initial responses to some of your points:

You'll need roxygen info for the modelInfo and .env args for some of the functions in LINPRED.R. I'm sure R CMD check will catch any other such cases.

Good point, actually this is one of those things that the check will not catch because the function is inside a list.

Some of the abbreviations in variable naming feel a bit awkward, in particular modMatNames. I'd vote for modelMatrixNames despite its length.

Fine with me to change it. It's not an argument that's likely to get used a lot so I don't think the length matters that much.

I'm pondering whether we should use the name priors rather than priorSpecs.

I think we had this idea previously and went with priorSpecs because we didn't want to have a macro called priors and an argument called priors. But now the macro is LINPRED_PRIORS, so maybe it's fine to have the argument be priors?

The formulaHandler functions all have basically the same roxygen and these are not user-facing anyway. I suggest a single roxygen block with the use of @alias so that all of them refer to the same roxygen.
Actually, beyond that, if these are not user-facing do they need to be exported, and if not, then we might not need roxygen anyway.

I had it in my head that the formula handlers needed to be exported based on the way LINPRED searches for them. But now that I think about it this may not be true. However I think maybe there's a potential for issues (i.e., the formula handlers not being found) if LINPRED is called by another package such as with nimbleOccu? Similar to the problems we had with scoping with macros more generally. Need to test this.

[kenkellner]

You'll need roxygen info for the modelInfo and .env args for some of the functions in LINPRED.R. I'm sure R CMD check will catch any other such cases.

Good point, actually this is one of those things that the check will not catch because the function is inside a list.

Thinking about this more, I wonder if we actually should not have roxygen entries for modelInfo and .env. They are arguments to the macro function, but they are used only internally by nimble to pass information and will never be used by the user. So, should we take advantage of the fact that R CMD check will not actually cross-check the arguments of the macros (because they're inside a list) and leave the arguments out to avoid confusing users?

[paciorek]

Regarding modelInfo and .env, my vote would be just to add roxygen for them and indicate they are for internal use. Otherwise, one of us down the road might forget this discussion and be trying to figure out why we didn't (have to) document them.

[paciorek]

A few more comments based on looking through the test files. I didn't look exhaustively, but I did try to use the tests to see if I could come up with corner cases.

• In setPriors, I would rename the eta category to be lkj_shape or lkjShape, so if one has a list of priors, it's more clear what that element is about.
• Can you add error trapping for LINPRED for cases where (i) a user forgets to provide the LHS and assignment and (ii) puts the LHS into the formula.
• In vignette and roxygen can you highlight that for a variable to be treated as categorical it needs to be an R factor.
• At risk of reopening old wounds, I think I prefer "noncentered" to "noncenter" as the LINPRED arg name, but I think we talked about this and perhaps we had a good reason for "noncenter"?
• If one has the "w/x" nested syntax, which adds the "w" random effects, it looks like both the x and w random effects are added together (a non-hierarchical representation) rather than the x random effects being centered on their respective w effect (a hierarchical representation). I think this is fine and don't want to complicate things by adding another parameterization, but I did want to check if my understanding was correct.

[kenkellner]

Definitely changing:

• Improve explanation of centering argument in roxygen
• Roxygen for modelInfo and .env macro arguments
• modMatNames --> modelMatrixNames
• LKJ default eta changed to 1
• Explain in vignette LINPRED_PRIORS section why it doesn't need to be used usually
• Don't export formulaHandlers
• Rename eta to lkjShape
• Error trap LINPRED when LHS is in formula
• Highlight in vignette the format for categorical variables (factor or character)
• Move vignette changes from nimbleMacros.Rmd to nimbleMacros.Rmd.orig

Proposed changes:

• priorSpecs argument --> priors?
• noncenter --> noncentered?

Change must be done in nimble (in buildMacro):

• Error trap missing LHS/assignment with LINPRED

[kenkellner]

• If one has the "w/x" nested syntax, which adds the "w" random effects, it looks like both the x and w random effects are added together (a non-hierarchical representation) rather than the x random effects being centered on their respective w effect (a hierarchical representation). I think this is fine and don't want to complicate things by adding another parameterization, but I did want to check if my understanding was correct.

This is (in my understanding) what lme4 does with this notation. For example:

y <- rnorm(1000)
x <- factor(sample(letters[1:3], 1000, replace=T))
w <- factor(sample(letters[4:6], 1000, replace=T))
dat <- data.frame(y=y, x=x,w=w)

library(lme4)
lmod <- lmer(y~1 + (1|w/x), data=dat)
summary(lmod)

Linear mixed model fit by REML ['lmerMod']
Formula: y ~ 1 + (1 | w/x)
   Data: dat

REML criterion at convergence: 2807.7

Scaled residuals: 
    Min      1Q  Median      3Q     Max 
-3.1589 -0.6721  0.0003  0.6980  3.6255 

Random effects:
 Groups   Name        Variance Std.Dev.
 x:w      (Intercept) 0.000000 0.00000 
 w        (Intercept) 0.002289 0.04784 
 Residual             0.965223 0.98246 
Number of obs: 1000, groups:  x:w, 9; w, 3

Fixed effects:
            Estimate Std. Error t value
(Intercept)  0.01860    0.04159   0.447
optimizer (nloptwrap) convergence code: 0 (OK)
boundary (singular) fit: see help('isSingular')

ranef(lmod)

$`x:w`
    (Intercept)
a:d           0
a:e           0
a:f           0
b:d           0
b:e           0
b:f           0
c:d           0
c:e           0
c:f           0

$w
   (Intercept)
d  0.008016817
e  0.026985551
f -0.035002368

Here's the equivalent nimble code from nimbleMacros:

library(nimbleMacros)
nimbleOptions(enableMacroComments=FALSE)
code <- nimbleCode({
  LM(y ~ 1 + (1|w/x))
})

mod <- nimbleModel(code, constants=as.data.frame(dat))

mod$getCode()

{
    for (i_1 in 1:1000) {
        y[i_1] ~ dnorm(mu_[i_1], sd = sd_residual)
    }
    for (i_2 in 1:1000) {
        mu_[i_2] <- beta_Intercept + beta_x_w[x_w[i_2]] + beta_w[w[i_2]]
    }
    beta_Intercept ~ dnorm(0, sd = 1000)
    sd_x_w ~ dunif(0, 100)
    for (i_3 in 1:9) {
        beta_x_w[i_3] ~ dnorm(0, sd = sd_x_w)
    }
    sd_w ~ dunif(0, 100)
    for (i_4 in 1:3) {
        beta_w[i_4] ~ dnorm(0, sd = sd_w)
    }
    sd_residual ~ dunif(0, 100)
}

[kenkellner]

• Can you add error trapping for LINPRED for cases where (i) a user forgets to provide the LHS and assignment and (ii) puts the LHS into the formula.

I added (ii), but (i) must be error trapped in nimble::buildMacro. The error occurs before LINPRED actually runs.

[paciorek]

Ok, for (i) can you put a PR into nimble when you have a chance?

[kenkellner]

PR is here.