Skip to content

Conversation

@perrydv
Copy link
Contributor

@perrydv perrydv commented Nov 25, 2025

Fixes #1599 and other changes.

This PR provides several updates to MCEM that are (generally relatively minor) bug fixes and cleanup.

  1. When a user provided a pStart vector, and if parameter boundaries were not being used (i.e. typically), the pStart was actually not being put in the model. So in effect the algorithm was starting from the values currently in the model, which was the default behavior anyway. In addition, there was no calculation of paramDeps (see next point) prior to the very first MCMC sampling.
  2. The determination of paramDeps, that is deterministic nodes between parameters and random effects, is updated to be the same as the recent updates to the same need in buildAGHQ (moving to package nimbleQuad). That is an improvement in efficiency.
  3. The burnin was not applied in a way consistent with the rest of nimble's MCMC systems. It was used as a post-thinning burnin instead of a pre-thinning burnin. (That could cause a confusing error because it could result in no samples being used for maximization (when one would reasonably expect there should be some) and an error from optim as a result.) That is changed / fixed.
  4. Furthermore, if the user provided a default thin interval as a control element to buildMCEM, it was not properly used. (The run-time argument thin to method findMLE was properly used at this point, but see the next point.)
  5. The use of reset to MCMC$run was not being properly applied. Accidentally it was used as the thin argument, again potentially resulting in a provided thin being ignored and also in the intended use of reset not occurring. When the algorithm to increase the sample size is triggered, instead of appending new samples, it was generating an entirely new MCMC sample, which was not wrong but was inefficient. This is fixed.
  6. When the MCMC sample size was increased, samples were generated given the last maximized parameters, but with the correction to use reset=FALSE and actually append samples, we needed to set the model parameters back to those used for the previous MCMC samples. This was not being done. In a sense this glitch and the previous glitch canceled each other: since we were generating entirely new samples, it was at least consistent with accidentally conditioning on different parameters, which should not have been wrong, just inefficient. It is possible this was all intended with the expectation of improving efficiency later. However, the values of reset suggest otherwise, and they were ending up in the wrong place.
  7. In vcov (the method for obtaining the variance-covariance matrix at the MLE), if resetSamples=TRUE, the newly generating MCMC samples were not using the thin interval. In addition the paramDeps were not being calculated. This typically would not have mattered because vcov would be called right after finding the MLE and the model would be up to date for the MLE parameters.
  8. The nimDerivs argument reset=TRUE (to re-tape with any new values of any constants, different than the MCMC$run reset discussed so far) was not being utilized. In most typical cases this would not matter because the AD tape constants will be the data values and those will not typically be changed. However, now the tape is reset upon the first use within a call to findMLE. This should be safer.
  9. There is a new nimbleOption to control whether buildMCMC emits a warning when not all nodes are being sampled, and buildMCEM uses that option to silence said warning since it normally builds an MCMC only for random effects, not parameters. Thanks @paciorek.
  10. There is a check for whether mcem is installed. Thanks @paciorek .
  11. Some messages are revised.
  12. Some regression tests for some of the above issues (when feasible, but a few were not really easy to set up tests for) were added to the end of test-mcem. A new version of the mcemTestLog_Correct.Rout "gold file" was generated.

Although this is kind of extensive, evidence that the changes aren't really that major is that none of the test outputs changed substantially. They changed in very minor ways due to different MCMC samples due to appending rather than fully resampling when M (sample size) is increased. The worst problems were not using pStart properly and not respecting thin settings in various ways. Other issues were about efficiency or were such that they would apparently only cause problems in particular use cases.

@paciorek
Copy link
Contributor

@perrydv your modification of the MCEM optimization failure message blew away my update to that message that tried to clarify things in response to issue #1535, namely "Please check the state of the compiled model and determine which parameter values are causing the invalid log-likelihood by calling 'calculate' with subsets of the model parameters (e.g., 'name_of_compiled_model$calculate('y[3]')' to see if node 'y[3]' is the cause of the problem)."

Did you intend to blow away that additional info?

@paciorek
Copy link
Contributor

@perrydv the test failures are very small changes in numerical values that affect the gold file comparison. I'm happy to fix those later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

buildMCEM gives spurious warning about unsampled nodes

3 participants