Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #395 +/- ##
==========================================
- Coverage 92.86% 92.52% -0.34%
==========================================
Files 31 31
Lines 3041 3105 +64
==========================================
+ Hits 2824 2873 +49
- Misses 217 232 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 32fad02 is merged into master:
|
|
Cool, I'll take a look soon |
|
Sol generated summary of the excpected benefits 1. ELPD MCSE calculation — high benefitAffected code: Herbie improved the scalar log-normal variance transformation from 35% to 100%. The original implementation converted likelihoods and expectations to linear scale. For sufficiently The new calculation operates on log-relative likelihoods and applies stable Expected benefits:
2. Weighted variances — high benefit for large offsetsAffected code:
Herbie could not improve the abstract scalar expression because it saw the first and second moments The empirical test exposed complete cancellation: The new implementation calculates weighted squared deviations from the weighted mean. Expected benefits:
3. Subsampling difference of squares — high local benefitAffected code: The broad Herbie sample scored both expressions at 100%, because random inputs rarely contain nearby For adjacent doubles near Expected benefits:
4. PSIS tail exceedances — moderate to high benefitAffected code: Herbie improved the analyzed exponential-difference kernel from 11% to 100%. For nearby log weights around The stable helper computes [ as [ with Expected benefits:
5. Stacking gradients — moderate benefitAffected code: Herbie strongly favored an For nearly identical model log predictive densities: The stable helper computes [ without directly subtracting nearby exponentials. Expected benefits:
|
|
@avehtari The benchmark results look a bit concerning. That's a pretty large slowdown:
Also, I was wondering if there were downsides to including these changes, so I asked Codex to review the PR looking for unintended consequences like performance regressions or other implications for existing functions and it found several things. I looked into its findings and verified most of them myself too. They're not all equally important, but some seem quite important. There are a few things that we should probably have tests for that would have caught some of these things beyond what the performance benchmarks already caught. Codex also found a pre-existing moment matching bug (see end of review) that we should probably fix as part of this PR. Here's what Codex found: The core numerical reformulations are mathematically sound for ordinary finite inputs, but the new helpers introduce two regressions for supported Blocking issues
Required changes
Computational costYes, there is a measurable cost, primarily from
So the end-to-end slowdown is moderate but real—not prohibitive, though worth explicitly accepting or optimizing. Adjacent pre-existing issue
This bug exists on both base and PR branches, so I did not count it as a regression, but this PR is a natural place to fix it and add coverage. All focused tests and current CI pass; the failures above require boundary cases not presently tested. |
|
Based on your Codex analysis the slowdown is not really a practical issue, but the blocking ones are clear. It's clear that our tests have been weak since these were not caught. I'll fix |
To me
Yeah this was useful also because it pointed out a few holes in our tests that we should add coverage for |
|
All Codex issues fixed and some more. Speed might be faster now, but let's see Touchstone results
|
Yeah speed looks much better! I'll take a look at the other changes soon. |
Herbie https://herbie.uwplse.org/ checks floating point computations, detects inaccurate expressions and finds more accurate replacements. I used Herbie with Sol to find these fixes. I have checked that all of them make sense.
Summary
loo.R: Compute pointwise ELPD Monte Carlo standard errors in log-relativecoordinates, preventing underflow and overflow for extreme log likelihoods.
E_loo.R,loo_moment_matching.R: Calculate weighted variances from centered observations instead ofsubtracting squared moments.
loo_subsample.R: Factor differences of squares in subsampling estimators to avoidcancellation for nearby large values.
psis.R: Useexpm1()when calculating PSIS tail exceedances from nearby log weights.loo_model_weights.R: Use a sign-awareexpm1()formulation for stacking gradients when modelpredictions are nearly identical.
Added focused regression tests for all five numerical issues.
Ran the complete test suite: 1168 passed, 0 failed.
There are three new helper functions which are used once or twice, and they could be also inlined, but I think using them improves the readability.