rewrite buildMCEM and associated tools and documentation

[perrydv]

This contains a nearly complete rewrite of buildMCEM. This includes:

• Parts of the model not connected to the latent states are now included in MLE calculations.
• Greater control and minor extensions over the ascent-based MCEM approach.
• setupMargNodes is used to determine default roles of nodes, which should provide the same handling as for buildLaplace.
• AD is used in the maximization if possible.
• buildMCEM is now a nimbleFunction. Previously it was an R function that created some nimbleFunctions but drove the algorithm from R instead of compiled code.

One thing I did not do (but could) is create a new predefined nimbleList class for results from MCEM. This would be useful, but I'm wondering if it's not really that important right now.

There is a very extensive roxygen draft entry. The User Manual section has been revised and shortened.

[perrydv]

@paciorek @danielturek It would be good to some other eyes on this at some point, even at least the documentation and API and names of user-facing things.

[paciorek]

Ok, I looked through some but not enough to really do code review of the algorithm details.

I'm not following the distinction in the testing between cases with calcNodesOther and without.

A couple minor things:

I'd prefer to just use our standard messageIfVerbose pattern in setup code rather than having a distinct verboseUse. If you really want the latter, I would like it to be named verbose and not verboseUse.

In findMLE we have "[Warning]" followed by stop. Given that, it's not a warning, so let's just omit " [Warning]" from the error message.

[danielturek]

@perrydv

To be consistent with the method name findMLE, would it make more sense to have the covariance-finding method be called findCov, rather than the current name vcov ?

How big a deal (from the package standpoint) is adding the addition Suggests for the mcmcse package, in DESCRIPTION? I know we try to keep the Imports to the minimum, so I'm wondering about Suggests. I guess it's unavoidable though? And is merely making it a Suggests field sufficient, being that the nimbleRcall to mcse is necessary for the algorithm?

What exactly happens in the case of discrete latent states (random effects)? In addition to emitting a warning in setupMargNodes, will something error out later? Or do we just remove them, and things continue, everything "works" in some sense? Just giving a warning that "this is likely to cause problems" feels unsatisfactory. Can we expand this, to state that discrete random effects will be omitted, and therefore the whole algorithm is being executed as though they are not in the model - and the results may not be what a user wants? (if that's correct).

I, as well, did not inspect the algorithm carefully.

I read through the roxygen for buildMCEM, and truthfully, it read pretty well, and I have no serious suggestions.

Line 592 of MCEM_build.R, I think "override" is a word in of itself (no hyphen required).

Lines 697 - 698, it wasn't clear to me on first reading what "is created by the same call with \code{config} instead of ..." meant. Looking at the code (how config is used) I now understand what this means, but without looking at the relevant code, I didn't understand this.

How would you feel about changing the continue argument (with default FALSE) to instead be reset (with default TRUE)?

Missing a period at the very end of line 353 in chapter_OtherAlgorithms.Rmd.

[perrydv]

Thanks @paciorek @danielturek. These will be mostly clarifications and some minor changes. I am on another branch as I write this so I'll post the comment and make the changes later.

• calcNodesOther: e.g., in the first test with that, using the pump model, notice that the latent nodes are entered as only theta[1:8], rather than all theta[1:10]. With fixed values in theta[9:10], these become part of the dependencies of the parameters that are not part of what is marginalized in the MCEM algorithm but do need to be included in the likelihood. That is what calcNodesOther is for. The test checks that these are detected and included in the calculations. (Lacking this feature was what caused a user issue and flagged the need for updates.)
• verbose: I would like to have a separate verbosity feature for MCEM, as it can be involved in its messaging potential. I think of this a bit like the separate progress bar setting for MCMC, although not with an actual progress bar. I think we talked about this at one point. Given that, I have a control argument called verbose. The name verboseUse is entirely internal. There is a system for a lot of the (many) run-time options to initialize them from the control list but then allow single-run updating of them when making a run-time call (to avoid rebuilding just to tweak run-time options). The Use suffix is a systematic part of the many internal names to keep those things straight. The user-facing name in the control list and a function argument is verbose.
• "[Warning]": nice catch. I will remove that.
• "vcov" vs "findCov". In my thinking findMLE is doing an optimization search and so "find" makes sense, but vcov is just calculating a vcov, not finding or searching in any algorithmic sense. I'm open to naming ideas but vcov seems somewhat common.
• Expansion of "Suggests" list: I am also cautious in keeping our package lightweight in terms of package dependencies. But MCEM needs a way to estimate ESS, and many packages do this so it seems silly and limiting to write our own, so my thinking is that we can go with a new Suggests entry. Open to other ideas.
• Discrete latent nodes: I am not following your thoughts here. MCEM actually can work fine with discrete latent nodes. Each iteration involves MCMC for latent nodes (which can be discrete) and then optimization of top-level parameters given a fixed sample of latent nodes (also no problem for discrete latents). It looks like I need to add a test for discrete latent nodes. (Also noting here "MCMC" in the last test title should be "MCEM")
• continue vs reset: I did think about this. Somehow the positive option of continue=TRUE seems more intuitive to me for MCEM than the negative option of reset=FALSE. MCMC can potentially run on and on while MCEM should in theory at some point be converged and done. continue=TRUE is like saying "This doesn't look done yet." I realize that is akin to reset=FALSE for MCMC, but continue has been my instinct. @paciorek do you have an opinion? I'm not clear how much consistency of argument naming (i.e., between MCEM and MCMC) matters here.
• Two typos and a clarification: thanks, will fix.

[paciorek]

I'm fine with continue for the reasons you give.

[danielturek]

Sounds good to me.

[perrydv]

Moved documentation notes and typos to #1434 so I can look at merge conflicts and merge this for @paciorek 's workflow.

[perrydv]

@paciorek Lots of test failures here are due to not having package mcmcse. I thought I had added it for testing by adding it to the list in install_requirements.R. Am I doing that wrong?

[paciorek]

It needs to be in Suggests in DESCRIPTION plus you'll probably need a requireNamespace("mcmcse") before you use the mcmcse function.

I'm happy to just deal with this all - how about you leave it for me to merge in.

[paciorek]

@perrydv I see you added @export for some new, undocumented functions for MCEM. Are you sure they need to be exported?

[paciorek]

The mcmcse-related test failure seems to have occurred because mcmcse failed to install on the runner. I'm playing with the install script.

[perrydv]

Just MCEM_mcse? That could get documentation to just say "see buildMCEM". Its purpose is to calculate the Monte Carlo standard error from an MCMC sample.

[paciorek]

It looks like we may need to only export MCEM_mcse, so that is what I am exporting now. I guess because that is called by a nimbleRcall?

[perrydv]

Yes, we call it from C++ and it needs to be findable in the global environment. I tell users in the documentation that they can provide a new MCMC_mcse in the global environment if they want to change methods.

[paciorek]

We now have an MCEM test failure on Windows only. It's a bit hard to tell if the issue is the lack of /usr/bin/time or an actual failure because the testing if printing out a ton of stuff and it's hard to see in the CI output.

It looks like @perrydv had not yet re-enabled our usual flags about verbosity and whatnot, nor reset the MCEM gold file (it's actually gone completely from new-MCEM). I can work on that and see if that leads me to determine the test failure is a non-issue or something to be dealt with.

[perrydv]

I did end up with a non-gold-file new MCEM testing system, IIRC. I will have to look. I don't know why numerical results would differ on Windows. Is the /usr/bin/time from run_tests.R ?

[paciorek]

Ok, I am in the middle of generating one. We should talk about this before you make any commits as I've been making changes to the branch and have some stuff in progress.

I don't think we want as much being printed to the screen as is currently happening because you left out the various preamble stuff (setting verbosity, no MCMC progress, sink, etc.) we have in many of our numerically-focused test files. Which to me implies we want to sink to a gold file.

[paciorek]

I will look into the timing thing as needed. I am suspecting the Windows error is a red herring.

[perrydv]

Sorry about the verbosity. We could also try to turn that back down with various settings.

[paciorek]

Canceling workflow given issues with the new mcem goldfile. I will hopefully look at this more tomorrow.

My inclination is that verbosity is fine (even good) provided in a gold file.

[paciorek]

Error appears to be a minor thing in gold file that I am fixing. I am merging this in rather than waiting for CI to run.