scheidan / adaptMCMC

R package that provides an implementation of the generic adaptive Monte Carlo Markov chain sampler proposed by Vihola (2011).
GNU General Public License v2.0
9 stars 5 forks source link

Allowing p to return a vector instead of a single value #1

Closed venelin closed 7 years ago

venelin commented 7 years ago

Dear Andreas,

Sorry for my long silence. I've implemented the change we've previously discussed. I'd super happy if this can quickly become part of a new adaptMCMC version on CRAN. Currently, my package POUMM is using a modified version of the MCMC function. It will be ideal if I can simply reference / include the adaptMCMC package.

Commited changes:

  1. change to mvtnorm-package to fix failing tests;
  2. In MCMC and MCMC.add.samples, return a matrix log.p as element in the returned list (parameter list = TRUE), when the user-specified function p returns a vector instead of a single number.
  3. updated documentation

p.s. I haven't updated any metadata in the package (e.g. NAMESPACE and DESCRIPTION).

Cheers, Venelin

scheidan commented 7 years ago

Hi @venelin, Thanks for your effort! I understand your motivation for this change. At the same time I think it may be confusing that a probability density is allowed to return a vector (we use this package for teaching too).

A clean option I can envision would be that the density function either returns a scalar (the normal case) or a named list containing at least an element log.p. The list returnd by MCMC would in both cases contain a vector for log.p and an optional 'extra outputs' list.

The advantages would be:

I try to give it a go this week, but I can't promise anything.

venelin commented 7 years ago

Hi Andreas,

I fully agree with your arguments in favour of a named list instead of a vector.

One reason I implemented the vector-based solution is that it allows me to ignore the moment when the new adaptMCMC version gets published on CRAN: My density function (returning a vector) works with the current CRAN version of adaptMCMC. Given that the current adaptMCMC version only stores the first element of the vector (i.e. the value log.p), my package does not report the log-likelihood values from the MCMC, but this is not an issue in the current POUMM version.

The reason I am making this clarification is to ask if possible for some backward compatibility: If possible, in the new version, please don't throw an error in case of a vector returned by the density - function - a warning that all but the first values in the vector are ignored would still be fine.

scheidan commented 7 years ago

The current version troughs a warning in case the density function returns a vector! Thinking about it should really be an error. I'd like rather to change that. I see two options that we don't break compatibility with you package:

venelin commented 7 years ago

I was aware about the work-around you propose, but was a bit reluctant to go for repeated prior calculation. But from design p.o.v., this seems more appropriate. In the next POUMM release, I'll update the code, so that the density function returns a single number.

scheidan commented 7 years ago

I've implemented the possibility to store extra values as a list, see 5c9c389. I think this should cover your use case, but it would be good if you could check it.

scheidan commented 7 years ago

All issues should be addressed by c64a56207bc5e5fed9c333c0024d2f27a8cdfaa1

Thanks for the inputs!