PEcAn.meta.analysis::approx.posterior test is broken

[ashiklom]

1. Error: approx.posterior function returns expected dataframe given test data (@test.approx.posterior.R#28)
no 'dimnames' attribute for array
1: approx.posterior(trait.mcmc, priors = prior.distns) at testthat/test.approx.posterior.R:28

testthat results ================================================================
OK: 10 SKIPPED: 0 FAILED: 1
1. Error: approx.posterior function returns expected dataframe given test data (@test.approx.posterior.R#28)

[infotroph]

I think what's happening here is that approx.posterior calls as.matrix(dat), which gets dispatched to coda:::as.matrix.mcmc.list, which is apparently registered as an S3 method but not exported, which means we can't call it directly with :: and instead we have to make sure the coda namespace is loaded before calling approx.posterior. The reason the test passes devtools::test but fails in devtools::check is that somehow or another test is loading coda but check isn’t.

I see twothree possible fixes:

• @import coda to add the whole coda namespace to the PEcAn.MA namespace. That brings the S3 methods along even though we can’t @importFrom them individually. Pro: Keeps namespace concerns in the NAMESPACE file, doesn't require any code changes. Con: Not evident to future maintainers which imported functions we actually use.
• Put requireNamespace("coda", quietly=TRUE) at the beginning of the approx.posterior code, thus ensuring the methods are available but (I think?) not risking any namespace collisions. Pro: clearly expresses that we need to be able to search the whole coda namespace but can't point to the thing we need from it. Con: mixes namespace concerns into package code, gets called repeatedly.
• Put requireNamespace("coda", quietly=TRUE) into a .onLoad() hook so it's called once whenever the PEcAn.MA namespace is loaded. Pro: clearer separation, only called once per session. Con: Haven't tested this one yet.

[mdietze]

Of the options listed, the 3rd sounds most promising. As a 4th alternative, could we change as.matrix(dat) to coda:::as.matrix.mcmc.list(dat)? It works at the command line. Don't know if it will not work during the tests.

[ashiklom]

I agree that 3rd suggestion is probably the best one. @mdietze solution will work, but is not recommended and will probably trigger at least a note and more likely a warning or error from R CMD CHECK.

I wish somebody in the R community would volunteer to maintain/overhaul the coda package. It's pretty ancient (still supports the S language!), poorly coded, fragile, and inefficient.

[infotroph]

I've implemented the .onLoad approach in #1966, but a bit more on the reasoning here:

It sounds weird to say "S3 methods declared but not exported", but the intuition (adapted from a Hadley comment in an SO thread I can't find at the moment) is that while they're public (we're not using anything that's supposed to be hidden), they're designed to be used in the context of the coda namespace. Or, shorter: Even if we know where to look without loading the namespace, the methods themselves are likely assuming that the namespace is loaded.