Misspecified N parameter in Poisson process generator - #22
Closed
joaohbbittar wants to merge 1 commit into
Closed
Conversation
RaulSimpetru
added a commit
that referenced
this pull request
Jul 11, 2026
…on process The Poisson inter-spike threshold was drawn as the average of N Exp(1) variates (-(1/N)*log(prod(U))), which is a Gamma(N, N) renewal process (ISI CV = 1/sqrt(N)), not Poisson. Only N=1 was ever correct, so the poisson_batch_size / N knob silently turned the "Poisson" drive into a progressively more regular Gamma process and duplicated the existing DD_Gamma path. - Generator now draws a single Exp(1) threshold (-log(1 - U), which also avoids a latent log(0) that could permanently silence a unit). Removes the batch parameter; keeps Ninit (RNG decorrelation, an orthogonal concern). - Drop poisson_batch_size from DescendingDrive__Pool and DD. Regular / Gamma firing is served by process_type="gamma", shape=N. - Rename the afferents' misnamed poisson_batch_size to shape (it is the Gamma shape of the AffIa/II/Ib generators, not a Poisson batch size) and fix docstrings that described it as a firing rate. - Migrate call sites: true Poisson where Poisson was intended; explicit process_type="gamma", shape=N where regular firing was deliberately used (behaviour-preserving). - Add tests/test_poisson_process.py: statistical goodness-of-fit that the process is Poisson (KS vs Exponential, chi-square on spike counts, Fano factor, serial independence) and that the Gamma path is a genuine Gamma of the requested shape (KS fit, shape recovery, wrong-shape rejection), through both the public DescendingDrive__Pool API and the raw generator. Supersedes #22, which analysed and documented the bug (analysis by @joaohbbittar).
Member
|
Thanks @joaohbbittar — your analysis nailed it. Confirmed empirically: at The fix is implemented and folded into #19 (commit 603b245):
Closing as superseded by #19. Thanks again for the careful writeup and the empirical validation — it made this a quick, confident fix. |
RaulSimpetru
added a commit
that referenced
this pull request
Jul 16, 2026
… parallelization (#19) Migrates docs to properdocs/mkdocs-gallery (executed gallery with inline plots on deploy), fixes the descending-drive 'Poisson' generator to be a true Poisson process (removes poisson_batch_size; afferents' poisson_batch_size renamed to shape), parallelizes the Watanabe Optuna example, and folds in the clinical SCI/PIC examples. Poisson bug identified by @joaohbbittar (#22).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nparameter (poisson_batch_size): incorrect documentation and undocumented Gamma process behaviourProblem
The parameter
N(exposed aspoisson_batch_sizeinDescendingDrive__Pool) is incorrectlydocumented in all three places where it appears. More importantly, there is a behavioural issue
that the documentation silently hides:
N > 1does not produce a Poisson process — it producesa Gamma process.
Empirical findings
This claim was tested in a standalone notebook,
test_poisson_batch_size_synchrony_and_gamma.ipynb(not committed here — see "Test notebook"below for why, and how to get it). For
N ∈ {1, 2, 5, 10, 20}, pooled ISI statistics fromsimulated
DescendingDrive__Poolpopulations match theGamma(N, N)prediction closely: CV(ISI)tracks
1/√N, and a KS test againstGamma(N, N)is not rejected while a KS test againstExponential(the true-Poisson null) is strongly rejected for everyN > 1. OnlyN = 1passesas Poisson. This confirms the derivation below empirically rather than just analytically.
How the parameter actually works
The Cython implementation generates each inter-spike threshold via the log-of-product method:
Because each
-log(Uᵢ) ~ Exp(1), the threshold is the average of N independent Exp(1)variates, which follows an Erlang(N, N) = Gamma(shape=N, rate=N) distribution.
ISI statistics as a function of N
The test notebook (see "Test notebook" below) confirms this prediction empirically for
N ∈ {1, 2, 5, 10, 20}(empirical CV(ISI) tracks1/√Nclosely, KS test againstGamma(N, N)not rejected, KS test against
Exponentialstrongly rejected forN > 1) — exact figures,statistics, and histogram/fit overlays are in the notebook rather than reproduced here.
The mean firing rate is preserved for all N, but ISI variance shrinks as 1/N. Increasing N
does not improve a Poisson approximation — it progressively replaces it with a Gamma process
trending toward perfectly regular firing.
Conflict with the existing Gamma pathway
The codebase already provides
DD_Gamma/process_type="gamma"for Gamma-distributed ISIs.DDwithN > 1implements the same process, silently and with an incompatibleparameterisation (here
rate = shape = Nalways;DD_Gammaallows them to vary independently).A caller setting
poisson_batch_size=16while usingprocess_type="poisson"is unknowinglyrequesting Gamma(16, 16) ISIs with CV ≈ 0.25.
Documentation errors
Three files need correction regardless of which behavioural fix is chosen.
cells.py—DDdocstring:Current description calls
Na "maximum firing rate in Hz", which has no connection to theimplementation.
Ndoes not affect rate at all.descending_drive.py—poisson_batch_size:Current description says higher values "improve statistical accuracy". This is wrong:
N=1isalready an exact draw. Larger N changes the distribution, not the accuracy of sampling from it.
_poisson_process_generator.pyx:Current description says higher values "may improve statistical properties" — undefined and
misleading for the same reason.
Possible resolutions — decision needed
Both options below require existing callers using
N > 1underprocess_type="poisson"tochange their call — neither is a free lunch on that front. What differs is where the surviving
capability lives, and how big the migration is.
Option A — Reject
N > 1forprocess_type="poisson"Either raise a
ValueError, or emit aUserWarning(and proceed with the existing, now-honestly-documented Gamma(N, N) behaviour), when
N > 1is passed alongsideprocess_type="poisson".Correct all docstrings to describe the actual Gamma(N, N) behaviour. A caller hitting this has a
one-argument fix: switch to
DD(process_type="gamma", N=16)(or, if warning rather than raising,no fix is strictly required, but the warning makes clear they're getting Gamma ISIs rather than
Poisson). The error variant is the stricter, more honest option; the warning variant is softer and
non-breaking, but lets a caller keep silently relying on Gamma-under-a-Poisson-label if they
ignore the warning — which is close to the status quo this PR is trying to fix.
Option B — Deprecate
NinDDentirelyRoute all Gamma-like behaviour through
DD_Gamma/process_type="gamma", which alreadyexists and is more flexible.
DDwould only ever generate a true Poisson process. A callerhitting this has to switch classes:
DD_Gamma(shape=16, rate=16)instead ofDD(process_type="poisson", N=16). Cleaner long-term API (no overloaded, partially-ignoredNparameter on
DD), but a larger migration than Option A's same-class argument swap.Files affected
myogen/simulator/neuron/_cython/_poisson_process_generator.pyxNin Parameters and Attributesmyogen/simulator/neuron/cells.pyN; remove false "maximum firing rate" claimmyogen/simulator/neuron/populations/descending_drive.pypoisson_batch_size; add guard or deprecation per chosen optionNone of the rows above are implemented in this PR; they're listed so a follow-up implementation
PR has a checklist once a resolution is agreed. The validation notebook used to produce the
"Empirical findings" above is not in this table since it isn't being committed — see "Test
notebook" below.
Test notebook
The numbers cited in "Empirical findings" above came from a notebook,
test_poisson_batch_size_synchrony_and_gamma.ipynb, that I wrote to check the Gamma-processclaim (and the decorrelation note) rather than just asserting them. It's not committed to this
PR:
this should live (there's no
tests/notebooks/or similar today), andpicking a location unilaterally felt like the wrong call for a discussion-only PR. Happy to add
it properly once maintainers have a preference — or to help establish that convention if there
isn't one yet.
the repo at all depending on how you'd want to treat this kind of artifact long-term.
It's available on request — I'm happy to attach it directly to this PR/issue (e.g. as a
comment attachment) or share it however's easiest, so you can re-run or inspect it without
having to take the empirical claims above on faith. It runs real
DescendingDrive__Poolsimulations via NEURON at a constant firing rate (isolating the generator's own statistics from
any drive-profile effects) and is already executed, with plots and summary tables, so it can be
read without re-running.
If maintainers decide on Option A or B above, a small permanent regression test (e.g. a
pytest-based KS-test assertion that
N = 1ISIs are Exponential) would likely be worth addingseparately in an implementation PR — happy to extract one from this notebook if useful.