Allow a continuous size (dispersion) in nbinom and nbinomMu - #16
Conversation
In the negative binomial's mean/dispersion parameterisation `size` is a real dispersion parameter, not a count, and stats::dnbinom() has always accepted a continuous value. `size` was stored in an Eigen::VectorXi and assigned with (int)(size), so the C API entry points -- which model solves and focei reach directly, bypassing the R-level assertions -- silently returned the log-likelihood at trunc(size) for size > 1 and aborted the process for 0 < size < 1. The truncation also made the log-likelihood a step function of size, so the dispersion could not be estimated even when its true value was an integer. Store `size` as a double, replace the `size > INT_MAX` bound with `size <= 0` (NB2 requires a strictly positive dispersion, and the integer bound is meaningless for a double), and relax the R assertions to assertNumeric(). llikBinom() is unchanged: there `size` is a number of trials and the integer restriction is correct. Two tests asserting NA for `size > INT_MAX` encoded the old restriction and now assert the stats::dnbinom() value, which the new code matches exactly; new tests cover continuous size and its derivatives.
`neg_binomial_2_lpmf()` requires a positive finite mean. Both nbinom entry points could reach it with an out-of-domain mean, and because `rxLlikNbinom()` / `rxLlikNbinomMu()` are `extern "C"` -- called directly by rxode2 solves and focei, with no handler in between -- the resulting C++ exception escaped and aborted the R process. Dropping the `size > INT_MAX` bound in the size/prob form opened a new case: `mu = size*(1-prob)/prob` can now overflow to `Inf` for a large finite `size` with a small `prob`, which the old bound had rejected. Guard the derived mean itself rather than re-bounding `size`, which also covers the pre-existing `prob == 0` (`mu = Inf`), `prob == 1` (`mu = 0`) and, via the C API, `prob` outside `[0, 1]` (`mu < 0`). The mean/dispersion form has the same hole for a non-positive `mu`; guard that too. Tests cover each case, and NEWS.md records the new `NA` returns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review summaryReviewed by Claude and independently by Antigravity (Gemini 3.1 Pro). Both rounds were run against a real build — this environment needed The core change is right. FindingsBoth reviewers converged on one theme: the mean handed to Stan can still be out of domain, and since
Fixed in b00650d by guarding the derived mean rather than re-bounding Notes, not blocking
Verification
Looks good to merge from my side. The one thing I'd flag for @mattfidler is timing rather than code: |
At prob == 1 (size/prob form) and mu == 0 (mean/dispersion form) the negative binomial collapses to a point mass at zero, and stats::dnbinom() gives a likelihood of 1 at x == 0 and 0 elsewhere. neg_binomial_2_lpmf() needs a strictly positive mean, so Stan cannot evaluate these points at all; the previous commit made them return NA to stop the C++ exception escaping rxLlikNbinom() / rxLlikNbinomMu() and aborting the R process. Fill in the value R would instead -- 0 at x == 0, -Inf otherwise -- and report the derivative, which does not exist at a point mass, as NA. An unusable gradient is better than an unusable log-likelihood: rxode2ll now matches R at every point where R defines one. Key the size/prob branch on prob == 1 rather than on the derived mean == 0. Those are not the same condition: size*(1-prob)/prob also underflows to exactly 0 for a denormal size with a prob well under 1, where the true log-likelihood is finite (about -745) rather than -Inf. That case stays NA, as before. Tests cover both degenerate points -- against stats::dnbinom(), for a continuous size, and through the R-level interface -- plus the denormal underflow that must not take the degenerate path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
llikNbinom()andllikNbinomMu()require an integersize. In the negativebinomial's mean/dispersion (NB2) parameterisation
sizeis a real dispersionparameter, not a count -- the pmf uses
gamma(size + x) / (gamma(size) * x!), andstats::dnbinom(x, size, mu)has always accepted continuoussize. Continuousoverdispersion is the norm in count models, so this blocks
nbinomMu()models innlmixr2.The R-level
checkmate::assertIntegerish(size)is only the visible half.sizewasalso stored in an
Eigen::VectorXiand assigned withN(0) = (int)(size), so theC-API entry points (
rxLlikNbinom/rxLlikNbinomMu) -- which rxode2 model solves andfoceicall directly, bypassing the R assertion -- silently truncated it.Current behaviour
R entry point, any non-integer
size:Model/solve path, which never reaches that assertion:
No warning -- the returned value is the log-likelihood at
trunc(size).For
0 < size < 1the truncation givessize = 0, Stan'sneg_binomial_2_lpmfthrows, and the exception crosses the
extern "C"boundary uncaught:Because the truncation quantises
size, the log-likelihood is a step function of thedispersion -- its derivative is zero almost everywhere:
This is why the dispersion cannot be estimated even when its true value is an
integer: any optimiser has to move
kthrough non-integer values, and there it seesno gradient.
Downstream, a plain
nlmixr2count model with a dispersion parameter cannot be fit:Across a sweep of simulated data sets (100 subjects, 10 observations each; true
kin {0.3, 0.7, 1.5, 3}, four replicates each, plus three
nbinom()size/prob fits),0 of 19 fits succeed on the current release -- including every replicate whose
true
kis exactly 3, for the reason above.Changes
src/llikNbinom.cpp,src/llikNbinom2.cpp: storesizein anEigen::VectorXdinstead of anEigen::VectorXi(struct field, constructor andllik_nbinom()/llik_nbinomMu()signatures), and assignN(0) = sizewithoutthe
(int)cast. Stan'sneg_binomial_2_lpmfalready accepts a real precisionargument, so nothing else in the likelihood or its derivative changes.
size < 0.0 || size > INT_MAXbecomessize <= 0.0.The
INT_MAXbound is meaningless oncesizeis a double, and NB2 requiressize > 0; non-positive values returnNA_REALrather than reaching Stan andaborting the process. The guard on
xis unchanged --xis a genuine count.R/llik.R:assertIntegerish(size, ...)becomesassertNumeric(size, lower = 0, finite = TRUE)inllikNbinom()andllikNbinomMu().assertIntegerish(x)is kept.llikBinom()is deliberately untouched: theresizeis a number of trials andthe integer restriction is correct.
Tests
Two existing tests asserted the old behaviour and are updated:
llikNbinomInternal returns NA for size > INT_MAXllikNbinomMuInternal returns NA for size > INT_MAXsize = 2^31is a valid dispersion; it was only out of range because it had to fitin an
int. Both now assert the value equalsstats::dnbinom(...), which it doesexactly. They are joined by new
size <= 0returnsNAtests.New regression tests cover continuous
sizefor both parameterisations: valuesagainst
stats::dnbinom(), anddMu/dProbagainst central finite differences.Verification
fxmatchesstats::dnbinom(x, size, mu = mu, log = TRUE)to a maximum absoluteerror of 3.6e-15 over a 112-point grid of
xin 0:6,sizein{0.3, 0.7, 1.5, 4.25} and
muin {0.5, 2, 5, 20}; the size/prob form matchesstats::dnbinom(x, size, prob = prob, log = TRUE)equally well.dMuanddProbmatch central finite differences of the continuous-sizelogdensity to ~1e-9.
The documented integer-size examples (
llikNbinomMu(46:54, 100, 40),llikNbinom(46:54, 100, 0.5)) are unchanged.The solve path no longer aborts for
0 < size < 1, returns the continuous-sizevalue to 8.9e-16, and no longer agrees with the truncated-
sizereference.tests/testthat/test-llik.R: 44 blocks, 68 expectations, 0 failures, 0 errors.The log-likelihood is once again strictly monotone in the dispersion: over the same
kgrid that produced 3 distinct values, all 9 are distinct, no slope is zero, andeach matches
stats::dnbinom()exactly.The
foceisweep described above -- same seeds and data that fail 19 out of 19 onthe current release -- converges 19 out of 19 against the patched build, all with
relative convergence (4). Mean estimatedkover four replicates:kThe three
nbinom()size/prob fits recover 0.611, 2.399 and 2.959 against truevalues of 0.7, 2.5 and 3.0.
Built and tested on Windows, R 4.5.3.
Note on NEWS.md
The
NEWS.mdchange is purely additive, under the current2.0.16section; thereleased
2.0.15section is untouched. ItsINT_MAXbullet listssizealongsidexfor all five discrete distributions, which after this change is accurate only forrxLlikBinom-- rather than edit a shipped entry, the new bullet states where theINT_MAXbound still applies.