Add percent divergence and percent max treedepth (#21)

[StefanoMezzini]

fixes #21. changes:

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

[StefanoMezzini]

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

[sebdalgarno]

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.

[sebdalgarno]

1. embr sets the new option as mb.prop_divergent, but in glance.cmstand_mcmc_analysis you are grabbing mb.perc_divergent so the set option is never found and 0.2 is always just used as the backup.
2. version bump news file doesn't match the version in description. Just bump description to match 0.0.0.9001. Soon i will just do the minor bump as the API is pretty well established now.
3. need to do some validation on the max_perc_divergent argument value within glance - use chk.
4. you should add a test that would have caught this option name mismatch. i.e. try setting the option and then seeing that convergence actually changes. something like

stub <- stub_analysis(c(1, 0, 0, 0), c(0, 0, 0, 0), TRUE)

withr::with_options(list(mb.prop_divergent = 0.002), {
  expect_true(glance(stub)$converged)
})
withr::with_options(list(mb.prop_divergent = 0), {
  expect_false(glance(stub)$converged)
})

[StefanoMezzini]

2 fixed in b40df71
1 and 3 fixed in 2633667
4 fixed in ab1e0aa