BayesianTools icon indicating copy to clipboard operation
BayesianTools copied to clipboard

Get sample warnings inconsistent

Open florianhartig opened this issue 5 years ago • 3 comments

If numSample > available samples, getSample only throws a warning if thin = 1

Also, check if thin and numSamples interact correctly

florianhartig avatar Jul 16 '18 08:07 florianhartig

It works correctly. getSample help:

numSamples sample size (only used if thin = 1). If you want to use numSamples set thin to 1.

I think we should leave it this way. Combining thin and numSamples might only confuse people.

MaximilianPi avatar Sep 24 '18 13:09 MaximilianPi

I noticed some additional issues which seem suitable for consideration here.

This bit of getSamples() fires once for each chain, regardless of whether numSamples is an integer multiple of the number of chains. Given the if(i == 1) bit I'm not sure how this happening so it probably merits a look in the debugger (I'm assuming the current source is included in 0.1.7 build of the package since classMcmcSampler.R was last edited on the day of package release).

      if(thin == 1 && !is.null(numSamples)){
        nSamplesPerChain <- ceiling(numSamples/length(sampler$chain))
        
        if(i == 1){
         if(nSamplesPerChain*length(sampler$chain) > numSamples) warning("Due to internal chains, numSamples was rounded to the next number divisble by the number of chains.", call. = FALSE)
        }
        
        temp <- sampleEquallySpaced(temp, nSamplesPerChain)
      }

It seems sufficient to indicate this warning only once and only when ceiling() actually converts nSamplesPerChain to an integer. A fix would therefore probably

  1. Fix things so if(i == 1) behaves as expected or just hoist the check out of the loop over chains where it's currently placed.
  2. Include a check numSamples/length(sampler$chain) %% 1 is meaningfully nonzero or an equivalent.

But there may additional considerations if sampler can contain chains of differing lengths.

There also appear to be counting mismatches within sampleEquallySpaced() which further confuse the warning. For example, if I ask for 999 samples from three chains, getSamples() returns a parameter array of 999 rows as expected. Asking for 1000 samples causes nSamplesPerChain to round to 334, so 1002 samples should be returned. Instead I get 1008. Asking for 1002 samples also triggers the warning and returns 1008 samples. This implies runMCMC(nrChains = 3) results in length(sampler$chain) being nine instead of three, so perhaps a sqrt() or such is missing.

twest820 avatar Jul 02 '21 15:07 twest820

I had to revert https://github.com/florianhartig/BayesianTools/commit/51a2ee0c334cf016bbc612493a328e8f4f219754, check in the future what the exact problem was and correct

florianhartig avatar Sep 10 '22 12:09 florianhartig