-
Notifications
You must be signed in to change notification settings - Fork 189
Naive approach for IID potential evaluation for score estimators #1508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1508 +/- ##
===========================================
- Coverage 89.70% 79.31% -10.40%
===========================================
Files 122 125 +3
Lines 9394 9828 +434
===========================================
- Hits 8427 7795 -632
- Misses 967 2033 +1066
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
e16d436
to
e5c8480
Compare
sbi/simulators/__init__.py
Outdated
from sbi.simulators.linear_gaussian import ( | ||
diagonal_linear_gaussian, | ||
linear_gaussian, | ||
true_posterior_linear_gaussian_mvn_prior, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change. Its unrelated, instead just import in the tests from sbi.simulators.linear_gaussian import ...
tests/score_log_prob_test.py
Outdated
) | ||
|
||
|
||
def _compute_ess(proposal_log_weights: Tensor, true_log_weights: Tensor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these two lines I would prefer not introduce a new function with a big docstring (except if its reused several times and then it should be in sbi.utils.
I would just move the calculations in the test above and add a one sentence inline comment.
tests/score_log_prob_test.py
Outdated
|
||
@pytest.mark.parametrize("num_dims", [1, 2]) | ||
@pytest.mark.parametrize("iid_batch_size", [1, 2]) | ||
def test_score_fn_log_prob(num_dims, iid_batch_size): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test will be quite expensive now as one would have to retrain on each combination.
Please:
- move the test to
linearGaussian_npse_test.py
- similar to
test_npse_iid_inference
use the fixturenpse_trained_model
(you can skip all the posteriors which are have a uniform prior
This fixture is only trained once and the used by all tests, avoiding this overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing looks great.
Just a few changes i.e. moving the tests and using corresponding pytest
fixture needs to be done.
…r_gaussian_mvn_prior in tests
e5c8480
to
f0da7e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, great effort. Looks good.
This is done, but I will block merging this for now as it will have conflicts with #1497.
Addresses the issue #1450
Used Effective Sample Score (ESS) to evaluate the log_probs returned by estimated posterior. This way log_probs are being checked both for single observation and iid observations case through the test added via the parameter -
iid_batch_size