Rename diagnostics$n_eff to diagnostics$ess with backward-compatible deprecation#327
Open
ishaan-arora-1 wants to merge 3 commits intostan-dev:masterfrom
Open
Rename diagnostics$n_eff to diagnostics$ess with backward-compatible deprecation#327ishaan-arora-1 wants to merge 3 commits intostan-dev:masterfrom
ishaan-arora-1 wants to merge 3 commits intostan-dev:masterfrom
Conversation
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 Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
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.
Hi! This PR implements the
n_eff→essrename discussed in #192, following the approach @jgabry outlined in the comments there.What this does
The
diagnosticslist inloo/psis/tis/sisobjects now usesess(effective sample size) as the canonical name instead ofn_eff, aligning with the terminology used by theposteriorpackage and the updated Rhat paper.Implementation approach
I went with the phased deprecation strategy from the issue discussion:
New
loo_diagnosticsS3 class — thediagnosticslist now has its own class, which was a prerequisite @jgabry identified for being able to define custom extractors.Both slots present — the constructor
loo_diagnostics(pareto_k, ess, r_eff)stores bothessandn_eff(set to the same value), so existing code that accessesn_effby position or viaunclass()won't break immediately.Deprecation warnings on
n_effaccess — custom$,[[, and[methods onloo_diagnosticswarn whenn_effis accessed, suggestingessinstead. This follows the exact same pattern used for the deprecated estimate extractors onlooobjects (the$.loo/[[.loo/[.loomethods inloo.R).Sync on assignment — custom
$<-and[[<-methods ensure that assigning to eitheressorn_effkeeps both in sync, so code that does things likeloo$diagnostics$n_eff[i] <- valueduring moment matching still works correctly.psis_n_eff_values()updated — now reads fromessfirst, falling back ton_efffor backward compatibility with older serialized objects.Internal column rename —
pareto_k_tablenow 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, updatedpsis_n_eff_valuesandpareto_k_tableR/importance_sampling.R— useloo_diagnostics()constructorR/loo.R— useloo_diagnostics()inloo.functionandlist2importance_samplingR/loo_approximate_posterior.R— sameR/loo_moment_matching.R— useessinternallyR/loo_subsample.R— reconstruct diagnostics properly during subsample operationsR/psis.R,R/tis.R,R/sis.R— updated roxygen docsNAMESPACE— registered new S3 methodsNEWS.mdentry addedNote on snapshots
The serialized snapshot in
tests/testthat/_snaps/psis.mdfor"psis results haven't changed"will need to be regenerated (viatestthat::snapshot_accept()) since the diagnostics structure now includes theessslot and theloo_diagnosticsclass. 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_effas-is — it's conceptually different (relative MCMC efficiency) and doesn't need renaming.Fixes #192