Skip to content

Comments

Rename diagnostics$n_eff to diagnostics$ess with backward-compatible deprecation#327

Open
ishaan-arora-1 wants to merge 3 commits intostan-dev:masterfrom
ishaan-arora-1:feature/rename-n_eff-to-ess
Open

Rename diagnostics$n_eff to diagnostics$ess with backward-compatible deprecation#327
ishaan-arora-1 wants to merge 3 commits intostan-dev:masterfrom
ishaan-arora-1:feature/rename-n_eff-to-ess

Conversation

@ishaan-arora-1
Copy link

Hi! This PR implements the n_effess rename discussed in #192, following the approach @jgabry outlined in the comments there.

What this does

The diagnostics list in loo/psis/tis/sis objects now uses ess (effective sample size) as the canonical name instead of n_eff, aligning with the terminology used by the posterior package and the updated Rhat paper.

Implementation approach

I went with the phased deprecation strategy from the issue discussion:

  1. New loo_diagnostics S3 class — the diagnostics list now has its own class, which was a prerequisite @jgabry identified for being able to define custom extractors.

  2. Both slots present — the constructor loo_diagnostics(pareto_k, ess, r_eff) stores both ess and n_eff (set to the same value), so existing code that accesses n_eff by position or via unclass() won't break immediately.

  3. Deprecation warnings on n_eff access — custom $, [[, and [ methods on loo_diagnostics warn when n_eff is accessed, suggesting ess instead. This follows the exact same pattern used for the deprecated estimate extractors on loo objects (the $.loo / [[.loo / [.loo methods in loo.R).

  4. Sync on assignment — custom $<- and [[<- methods ensure that assigning to either ess or n_eff keeps both in sync, so code that does things like loo$diagnostics$n_eff[i] <- value during moment matching still works correctly.

  5. psis_n_eff_values() updated — now reads from ess first, falling back to n_eff for backward compatibility with older serialized objects.

  6. Internal column renamepareto_k_table now uses "Min. ESS" as the internal column name (the print method was already displaying it as "Min. ESS" since use new k threshold #235, but the actual matrix column was still "Min. n_eff").

Files changed

  • R/diagnostics.R — new class, constructor, extractor methods, updated psis_n_eff_values and pareto_k_table
  • R/importance_sampling.R — use loo_diagnostics() constructor
  • R/loo.R — use loo_diagnostics() in loo.function and list2importance_sampling
  • R/loo_approximate_posterior.R — same
  • R/loo_moment_matching.R — use ess internally
  • R/loo_subsample.R — reconstruct diagnostics properly during subsample operations
  • R/psis.R, R/tis.R, R/sis.R — updated roxygen docs
  • NAMESPACE — registered new S3 methods
  • Tests updated across 7 test files
  • NEWS.md entry added

Note on snapshots

The serialized snapshot in tests/testthat/_snaps/psis.md for "psis results haven't changed" will need to be regenerated (via testthat::snapshot_accept()) since the diagnostics structure now includes the ess slot and the loo_diagnostics class. I didn't want to blindly accept snapshots without running the full test suite locally, so I left that for the CI / reviewer to handle.

What I didn't change

Per the issue discussion, I left r_eff as-is — it's conceptually different (relative MCMC efficiency) and doesn't need renaming.

Fixes #192

This addresses stan-dev#192 by renaming the `n_eff` slot in the diagnostics
list to `ess` (effective sample size), following the convention
established by the posterior package and the new Rhat paper.

Changes:
- Add `loo_diagnostics` S3 class with a constructor that stores both
  `ess` and `n_eff` (for backward compatibility)
- Add custom `$`, `[[`, `[` methods for `loo_diagnostics` that emit
  a deprecation warning when `n_eff` is accessed
- Add `$<-` and `[[<-` methods that keep `ess` and `n_eff` in sync
- Update all diagnostics creation sites to use `loo_diagnostics()`
- Update `psis_n_eff_values()` to read from `ess` first, falling
  back to `n_eff` for backward compatibility with old objects
- Rename internal column in `pareto_k_table` from `"Min. n_eff"`
  to `"Min. ESS"` (print method already displayed it as ESS)
- Register new S3 methods in NAMESPACE
- Update all internal code and tests to use `ess`
- Update roxygen documentation for psis, tis, sis, and loo objects
- Add NEWS.md entry

Fixes stan-dev#192
- Use unclass() in psis_n_eff_values to avoid triggering deprecation
  warning when falling back to n_eff internally
- Update pareto_k_table column name test expectation to "Min. ESS"
- Remove stale serialized snapshots (psis, loo_and_waic,
  loo_moment_matching) so they regenerate with the new diagnostics
  structure that includes both ess and n_eff slots
Run tests locally with TESTTHAT_ACCEPT=true to regenerate serialized
and text snapshots that changed due to the ess/n_eff diagnostics
restructuring. All 1173 tests pass with 0 failures and 0 warnings.
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 92.59259% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.71%. Comparing base (2883f25) to head (74ab82d).

Files with missing lines Patch % Lines
R/diagnostics.R 87.50% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
- Coverage   92.78%   92.71%   -0.07%     
==========================================
  Files          31       31              
  Lines        2992     3048      +56     
==========================================
+ Hits         2776     2826      +50     
- Misses        216      222       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Change n_eff to ESS

2 participants