Fix tascCODA always returning insignificant results#1016
Merged
Conversation
The arviz 1.0 migration (#911) recomputed the posterior median in `summary_prepare` via `posterior[var_names].median(...).to_dataframe().stack().reindex(summ.index)`. That `stack()` yields a tuple MultiIndex (e.g. `("A", "x", "n0", "alpha")`) that never matches arviz's string row labels (e.g. `"b_tilde[x, n0]"`), so every median silently became NaN. tascCODA decides credibility with `|median| > delta`, so NaN medians made every node effect non-credible regardless of the data. scCODA is unaffected because it selects effects via inclusion probabilities rather than the median. Compute the medians positionally instead, which matches `az.summary`'s row order (by `var_names`, then C-order of each variable's dims) exactly. Also fix `run_hmc`, which passed a JAX PRNG key into `set_init_mcmc_states` where `np.random.default_rng` expects an int seed, raising `TypeError`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Lukas Heumos <lukas.heumos@posteo.net>
2602efb to
6fa34b6
Compare
060551b to
30a4912
Compare
Two independent breakages in the v6 upload step:
1. Auth: `use_oidc: true` needs an `id-token`, which GitHub won't issue for
fork PRs, while tokenless upload is rejected for the repo's own branches
("Token required because branch is protected"). The only config covering
both is token auth with a tokenless fork fallback: pass CODECOV_TOKEN, which
authenticates same-repo runs, while fork PRs get an empty secret and fall
back to tokenless (the case tokenless is actually allowed). Drop `use_oidc`.
2. GPG: intermittent "Can't check signature: No public key" came from the
uploader fetching its signing key from Codecov's deleted keybase account.
v7 migrates the key to `codecovsecops`, fixing verification at the source.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
30a4912 to
70c4689
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1016 +/- ##
==========================================
+ Coverage 73.54% 77.81% +4.26%
==========================================
Files 48 50 +2
Lines 5613 6580 +967
==========================================
+ Hits 4128 5120 +992
+ Misses 1485 1460 -25
🚀 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.
What
Fixes tascCODA always returning insignificant results, plus a
run_hmccrash.1. NaN node-effect medians (the main bug)
The arviz 1.0 migration (#911) recomputed the posterior median in
summary_preparevia:.to_dataframe().stack()produces a tuple MultiIndex (e.g.("A", "x", "n0", "alpha")) that never matches arviz's string row labels (e.g."b_tilde[x, n0]"), soreindex(summ.index)fills the entiremediancolumn with NaN.tascCODA decides node credibility with
|median| > delta(__complete_node_df), so NaN medians made every effect non-credible regardless of the data — i.e. "always insignificant".scCODA is unaffected because it selects effects via inclusion probabilities, not the median, which is why only tascCODA broke.
The fix computes the medians positionally, which matches
az.summary's row order exactly (rows ordered byvar_names, then C-order of each variable's dims — identical toDataArray.values.flatten()).2.
run_hmcrng-keyTypeErrorrun_hmcconvertedrng_keyto a JAX key before handing it toset_init_mcmc_states, which seeds a NumPy generator and therefore needs an int — raisingTypeError: SeedSequence expects int ....It now keeps the int seed for the NumPy generator and derives the JAX key separately, mirroring
run_nuts.Not included here
While verifying this, I found a deeper, separate problem: even with medians fixed, the (correct) spike-and-slab model recovers no credible effects because the global
thetacollapses to its prior under numpyro's samplers. That needs a model-level change and is tracked in #1015 with the full analysis.Tests
No new test is added: the meaningful assertion (tascCODA recovers known credible effects) is blocked by #1015, and asserting only that medians are non-NaN isn't worth pinning.
Existing
tests/tools/_coda/test_tasccoda.pypasses, andrun_hmcnow runs end-to-end.