nimble-dev / nimble

The base NIMBLE package for R
http://R-nimble.org
BSD 3-Clause "New" or "Revised" License
158 stars 24 forks source link

integer overflow in `nimbleGraph::getDependencyPathCountOneNode` #1321

Closed paciorek closed 1 year ago

paciorek commented 1 year ago

A user reported this error (from configurMCMC in checkConjugacy_new):

Error in if (max(numPaths) > sum(numDeps)) { : 
  missing value where TRUE/FALSE needed

The key part of the model structure causing this is

  P_sm[1] <- p[1]
  for(i in 2:I_nDays_min1){
    P_sm[i] <- (1-sum(P_sm[1:(i-1)])) * p[i]
  }
  P_sm[I_nDays] <- 1-sum(P_sm[1:90])

where I_nDays=91.

The recursive summation here causes the number of dependent paths to explode and quickly hit the maximum integer when considering the number of dependent paths for each p[i] (starting with p[91] the number grows very quickly).

I think the best solution is:

  1. Add a max arg to nimbleGraph::getDependencyPathCountOneNode so that it bails when it gets to that value, returning that value.
  2. This is only used in conjugacy checking. I think we can save a bit of computation by having max be set equal to sum(deps) since that is what the result of the path counting is going to be compared against. As soon as we know that any of the values of numPaths is that big, we know what to do in terms of what approach to conjugacy checking to use.
  3. In fact, if we wanted to use a for loop instead of sapply, we could bail out of the loop rather than running getDependencyPathCountOneNode for all the input nodes. Given the comment below and that we can cut things off when we get to sum(deps), which generally won't be that large, in most cases we won't save time, but it's probably worth doing for cases where we run nimbleGraph::getDependencyPathCountOneNode many times.

Interestingly even when the count gets as high as the max integer, it only takes 30 microseconds to run the recursion in getDependencyPathCountOneNode so efficiency is not much concern here if the function is only run a limited number of times.

I'm surprise we haven't run into this before given this occurred with only 90 elements.

@danielturek @perrydv I welcome any comments here.

paciorek commented 1 year ago

Fixed by PR #1322