Skip to content

Add percent divergence and percent max treedepth (#21) - #33

Open
StefanoMezzini wants to merge 19 commits into
mainfrom
add-percent-diverg-21
Open

Add percent divergence and percent max treedepth (#21)#33
StefanoMezzini wants to merge 19 commits into
mainfrom
add-percent-diverg-21

Conversation

@StefanoMezzini

Copy link
Copy Markdown
Contributor

fixes #21. changes:

  • updated roxygen and docs
  • added columns to the glance() output for percent divergence and percent max treedepth

@StefanoMezzini
StefanoMezzini marked this pull request as draft August 3, 2026 16:00
@StefanoMezzini
StefanoMezzini marked this pull request as ready for review August 3, 2026 16:14
@StefanoMezzini
StefanoMezzini marked this pull request as draft August 5, 2026 17:42
@StefanoMezzini

Copy link
Copy Markdown
Contributor Author

keeping in draft form until the conversation in #21 is resolved.

@sebdalgarno

Copy link
Copy Markdown
Member

I've reviewed and agree with these blocking issue from claude review:
1 + 2+ 3 are documentation issues - in some cases, preexisting, but have been exacerbated and should be fixed in this PR
4 is a good point - we should use our analysis_mode options as we do with rhat and esr. I suggest thresh of 0 (no divergences) for "paper" and 0.2 for all other modes. note this will have to be an addition to embr package.
5 + 6 are test issues where the tests aren't quite testing what you think. when analysis_mode is 'check' the convergence will always be FALSE (from rhat and esr) so not actually testing divergence convergence.

Critical Issues (Blocking)

1. R/glance.R:47 — the converged contract changed and the docs still describe the old one.

gl$converged <- gl$converged & (gl$perc_divergent < 0.2)

R/glance.R:18 still reads:

▎ converged: Logical indicating convergence (TRUE if max R-hat < rhat threshold)

That was already incomplete before this PR — embr::glance.mb_analysis() calls converged(x, rhat = rhat, esr = esr), so ESS ratio was always part of it — and this PR makes it a third of the truth. A user reading converged == FALSE will go hunting through R-hat values and find nothing wrong. Downstream code in the embr ecosystem that keys off converged now changes behaviour with no signal at all.

Fix the @return entry to state all three criteria, and add a NEWS.md bullet. NEWS.md currently has no entry for any of this despite two user-visible breaking changes (column rename, converged semantics).

2. R/glance.R:47 vs R/glance.R:20,29-30 — the code and the documentation say opposite things.

The docs, in two places, say divergences should be zero:

▎ perc_divergent: Problem indicators: Any value > 0% indicates sampling issues
▎ Divergent transitions: Should be 0. Any divergent transitions indicate the sampler had numerical issues and results may be unreliable.

The code declares a fit converged with up to 0.2% divergences. Both cannot be the stated position of this package. Either the threshold is wrong or the docs are. Pick one and make them agree — and if you keep the tolerance, the @details needs a sentence explaining why a nonzero rate is tolerated, because it contradicts standard Stan guidance.

3. R/glance.R:31-33 — the treedepth advice is wrong, and this PR touched the line without fixing it.

▎ Max treedepth: Should be 0 or very low. High values suggest the sampler is working hard and may benefit from increased adapt_delta or reparameterization.

Increasing adapt_delta shrinks the step size, which makes trajectories longer and treedepth saturation more frequent — it is the standard remedy for divergences, not for treedepth. The fix for treedepth is raising max_treedepth (and, more usefully, reparameterizing). This PR edited exactly this bullet to append "or reparameterization" while leaving the incorrect advice in place. Reword to increased max_treedepth or reparameterization.

4. R/glance.R:47 — 0.2 is a hardcoded magic number in a package where every other threshold is a user option.

rhat and esr come from getOption("mb.rhat", 1.1) / getOption("mb.esr", 0.33) and are overridable via .... The divergence threshold is neither. Worse, its practical strictness swings by two orders of magnitude across embr analysis modes, because it is a percentage of the saved draws and niters × nchains is mode-dependent:

┌───────────────┬──────────────────┬───────┬───────────────────────┐
│     mode      │ nchains × niters │ draws │ divergences tolerated │
├───────────────┼──────────────────┼───────┼───────────────────────┤
│ quick / debug │ 2 × 10           │ 20    │ 0 (1 divergence = 5%) │
├───────────────┼──────────────────┼───────┼───────────────────────┤
│ check         │ 2 × 500          │ 1000  │ 1                     │
├───────────────┼──────────────────┼───────┼───────────────────────┤
│ report        │ 3 × 500          │ 1500  │ 2                     │
├───────────────┼──────────────────┼───────┼───────────────────────┤
│ paper         │ 4 × 1000         │ 4000  │ 7                     │
└───────────────┴──────────────────┴───────┴───────────────────────┘

So the same model with the same problem is "not converged" in quick mode and "converged" in paper mode. That is a worse property than the count-based rule it replaces. Follow the existing convention:

perc_divergent = getOption("mb.perc_divergent", 0.2)

as a formal argument, documented with @param, and defend the default in the docs.

5. tests/testthat/test-zzz-analyse-mcmc.R:288-290 — this test passes for the wrong reason and tests nothing.

# not converged due to other reasons besides divergences
glance <- glance(stub_analysis(rep(250, 4), rep(250, 4), FALSE))
expect_identical(glance$converged, FALSE)

The mocked parent method at line 247 hardcodes converged = TRUE and ignores x entirely. The converged argument to stub_analysis() is stuffed into diag_summary — a list glance.cmdstan_mcmc_analysis() reads only num_divergent, num_max_treedepth, and ebfmi from. It is dead. This case asserts FALSE perc_divergent == 100, which is exactly what the previous block already tested. Delete theconverged parameter, or make the mock actually honour it (e.g. read it off an attribute on x) so the "converged was already FALSE stays FALSE" path is genuinely exercised.

6. tests/testthat/test-zzz-analyse-mcmc.R:353 — the funnel test's convergence assertion is vacuous.

expect_identical(glance$converged, FALSE)

This test runs under set_analysis_mode("check"), which sets mb.rhat = 1 and mb.esr = 1. R-hat is bounded below by 1, so max(rhat) < 1 is never satisfiable and converged is FALSE in check mode for every model,
divergences or not. This assertion would pass identically if line 47 were deleted. It givee integration path is covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add percentage of divergent transitions to convergence flag

2 participants