Skip to content

Handle errors of the user functions in loo_moment_match() - #410

Open
avehtari wants to merge 2 commits into
masterfrom
moment-match-errors
Open

avehtari wants to merge 2 commits into
masterfrom
moment-match-errors

Conversation

@avehtari

Copy link
Copy Markdown
Member

Fixes #274.

This PR was Claude assisted

Summary

loo_moment_match() calls log_prob_upars() and log_lik_i_upars() on transformed draws, which can be more extreme than the posterior draws, and the model code can throw an error for them. In #274, neg_binomial_2_rng in the generated quantities block overflows, because cmdstanr's log_lik_i_upars() runs the whole block to get log_lik.

The split step at the end called log_lik_i_upars() without try(), so an error there stopped loo_moment_match(). With cores > 1 and two or more observations, parallel::mclapply() returned the error as a string, and the result loop then failed with "$ operator is invalid for atomic vectors".

In the transformation loop, try() printed the error, and the loop stopped for that observation. The user got no warning that moment matching had stopped early.

Change

  • If the split step fails, the observation gets the result without the split, the same as with split = FALSE. The accuracy warning that split = FALSE gives for a large Pareto k of the self-normalized weights now also covers these observations, with a text that refers to the failed split.
  • If anything else fails for one observation, for example log_lik_i(), that observation keeps its PSIS results. A try-error from a mclapply() worker gets the same treatment.
  • The loop calls try() with silent = TRUE. One warning lists the observations for which moment matching stopped early or failed, and shows the first error message.

cmdstanr cannot avoid the error: its log_lik_i_upars() needs the generated quantities block to compute log_lik, and the block also runs the *_rng calls. Users can move the *_rng code to a standalone generated quantities run.

Tests

Four new tests in tests/testthat/test_loo_moment_matching.R use the existing test model, with a log_lik_i_upars() or log_lik_i() that fails in chosen calls:

  • The split step fails: both warnings appear, and the results equal those with split = FALSE.
  • The accuracy warning uses the new text after a failed split, and the old text otherwise.
  • The first transformation fails: the warning shows the error message, and Pareto k keeps its PSIS value.
  • log_lik_i() fails for observation 1, with k_threshold = 0 so that 30 observations go through moment matching, once with cores = 1 and once with cores = 2. Observation 1 keeps its PSIS results, and the other observations get the same results as without the error.

With the master R/loo_moment_matching.R, all four tests fail: three stop with an error, and one fails an expectation. With this PR, devtools::test() with NOT_CRAN=true passes all 1280 tests and gives no warnings (R 4.6.1, testthat 3.3.2, Linux x86_64).

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.50%. Comparing base (fd910fe) to head (8db33ac).

Files with missing lines Patch % Lines
R/loo_moment_matching.R 94.11% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
+ Coverage   92.48%   92.50%   +0.02%     
==========================================
  Files          31       31              
  Lines        3139     3204      +65     
==========================================
+ Hits         2903     2964      +61     
- Misses        236      240       +4     

☔ View full report in Codecov by Harness.
📢 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.

@github-actions

Copy link
Copy Markdown
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8db33ac is merged into master:

  • ✔️loo_function: 1.03s -> 1.03s [-1.44%, +1.59%]
  • ✔️loo_matrix: 1.18s -> 1.18s [-2%, +1.66%]
    Further explanation regarding interpretation and methodology can be found in the documentation.

@avehtari avehtari added the bug label Oct 21, 2026

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

neg_binomial_2_rng can obstruct loo_moment_match()

2 participants