Improve floating point accuracy - #395
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #395 +/- ##
==========================================
- Coverage 92.86% 92.50% -0.37%
==========================================
Files 31 31
Lines 3041 3107 +66
==========================================
+ Hits 2824 2874 +50
- Misses 217 233 +16 ☔ 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 31f5a11 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. |
jgabry
left a comment
There was a problem hiding this comment.
Looks good!
I made one comment about mcse_elpd but it's probably not a big deal. Besides that I noticed a couple of small things that aren't technically caused by this PR but could be good to fix here since we're touching code related to them. But it's also fine if you want to defer these for later. Just these two:
-
loo() accepts
+Inflog likelihoods which results inNAs. Should we just error for this case. We can still accept-Inflog ratios for PSIS, but+Inflog likelihoods shouldn't be allowed, right? Or am I forgetting a reason for that? -
stacking_weights()andpseudobma_weights()acceptNA,NaN,+Inf. Not a huge deal since the optimizer will error or the weights will come back NA and the user will know something's wrong. But maybe we should just throw an error ourselves?
|
|
It looks like the new |
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.