Skip to content

fix: ImproperEmpirical is inefficient#1700

Merged
janfb merged 1 commit intomainfrom
1635_improper_empirical
Nov 14, 2025
Merged

fix: ImproperEmpirical is inefficient#1700
janfb merged 1 commit intomainfrom
1635_improper_empirical

Conversation

@gmoss13
Copy link
Contributor

@gmoss13 gmoss13 commented Nov 14, 2025

Addresses #1635

As mentioned in #1635, ImproperEmpirical currently saves all the prior samples. However, these samples are never actually used, and ImproperEmpirical itself is only used for single round, posterior-estimation methods (so NPE, NPSE, MNPE, etc.), and even when it is used, we turn off mcmc_transform. We currently warn the user if the number of samples is big, that the memory usage will be large.

This PR makes some changes to ImproperEmpirical:

  • Instead of saving all the samples, it simply computes the mean and standard deviation of the samples
  • It allows mcmc_transform to z-score the parameters even when the prior is ImproperEmpirical (this was turned off before)
  • It raises a warning if sample() or log_prob() is called for ImproperEmpirical. The first we have to turn off if we don't want to save the samples. In principle, we can keep the current behaviour for log_prob of returning zeros for any value, but this is a bit weird for me - an empirical prior is unlikely to actually be uniform (improper), and if we need to call log_prob(), we would want the values to reflect what the log prob actually is (so either a proper prior, or to use the pyro.Empirical if we don't know what the prior is, which would be a mixture of delta distributions). Raising this warning did not create any test failures, because we never call log_prob() when using ImproperEmpirical

@codecov
Copy link

codecov bot commented Nov 14, 2025

Codecov Report

❌ Patch coverage is 79.41176% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.66%. Comparing base (06f13a8) to head (cce6553).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
sbi/utils/sbiutils.py 79.41% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1700      +/-   ##
==========================================
- Coverage   88.48%   84.66%   -3.82%     
==========================================
  Files         137      137              
  Lines       11470    11484      +14     
==========================================
- Hits        10149     9723     -426     
- Misses       1321     1761     +440     
Flag Coverage Δ
unittests 84.66% <79.41%> (-3.82%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
.../inference/potentials/posterior_based_potential.py 84.00% <ø> (+4.75%) ⬆️
sbi/inference/trainers/npe/npe_base.py 93.54% <ø> (+0.63%) ⬆️
sbi/utils/sbiutils.py 79.68% <79.41%> (-8.12%) ⬇️

... and 26 files with indirect coverage changes

Copy link
Contributor

@janfb janfb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for looking into this @gmoss13 !

@janfb janfb merged commit 2c216c2 into main Nov 14, 2025
9 checks passed
@janfb janfb deleted the 1635_improper_empirical branch November 14, 2025 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants