Skip to content
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

Evaluation of posterior fit #1023

Merged
merged 23 commits into from
Apr 2, 2024
Merged

Evaluation of posterior fit #1023

merged 23 commits into from
Apr 2, 2024

Conversation

Ziaeemehr
Copy link
Contributor

@Ziaeemehr Ziaeemehr commented Mar 19, 2024

Add to function for evaluation of posterior fit.

Fixes issue #1017

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.

  • I have read and understood the contribution
    guidelines
  • I agree with re-licensing my contribution from AGPLv3 to Apache-2.0.
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • [] I have reported how long the new tests run and potentially marked them
    with pytest.mark.slow.
  • New and existing unit tests pass locally with my changes
  • I performed linting and formatting as described in the contribution
    guidelines
  • I rebased on main (or there are no conflicts with main)

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 76.36%. Comparing base (4c2f71e) to head (4e3584d).
Report is 25 commits behind head on main.

❗ Current head 4e3584d differs from pull request most recent head d3988c3. Consider uploading reports for the commit d3988c3 to get more accurate results

Files Patch % Lines
sbi/analysis/sensitivity_analysis.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1023      +/-   ##
==========================================
- Coverage   76.37%   76.36%   -0.02%     
==========================================
  Files          84       84              
  Lines        6507     6512       +5     
==========================================
+ Hits         4970     4973       +3     
- Misses       1537     1539       +2     
Flag Coverage Δ
unittests 76.36% <60.00%> (-0.02%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Ziaeemehr Ziaeemehr changed the title add posterior shrinkage and zscore. Evaluation of posterior fit Mar 19, 2024
@@ -501,3 +502,53 @@ def project(self, theta: Tensor, num_dimensions: int) -> Tensor:
projected_theta = torch.mm(theta, projection_mat)

return projected_theta


def posterior_shrinkage(prior_std, post_std):
Copy link
Contributor

@manuelgloeckler manuelgloeckler Mar 20, 2024

Choose a reason for hiding this comment

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

Currently, the function does not do much work.

Maybe change inputs to prior and posterior samples and do the estimation of the standard deviation within?

Copy link
Contributor

Choose a reason for hiding this comment

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

Type hints should also be here and not in the docstring.


Parameters
----------
prior_std : float, array-like
Copy link
Contributor

Choose a reason for hiding this comment

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

Typing should also be valid for tensors (i.e. vectors)

I guess one can define this metric also in a multivariate setting, just with the "total stddev" i.e. the sum.

----------
prior_std : float, array-like
The standard deviation of the prior distribution.
post_std : float, array-like
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

return 1 - (post_std / prior_std) ** 2


def posterior_zscore(true_mean, post_mean, post_std):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above i.e. type hints here.

And the metric should be suitable for vector input.

Copy link
Contributor

@manuelgloeckler manuelgloeckler left a comment

Choose a reason for hiding this comment

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

Hey. I left a few comments, let me know if something is unclear.

Not sure if it also is in the right folder, maybe move them to the metrics.py in utils?

@Ziaeemehr
Copy link
Contributor Author

@manuelgloeckler Thanks for the comments.
pls let me know if there is any other suggestions

@janfb janfb requested a review from manuelgloeckler March 21, 2024 16:52
Copy link
Contributor

@manuelgloeckler manuelgloeckler left a comment

Choose a reason for hiding this comment

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

Great, looks good.

Just a few signature type hints are missing.

And I would stick with torch, if we do not have to use numpy. Our posteriors return torch.Tensors so lets also stick with it here.

Copy link
Contributor

@manuelgloeckler manuelgloeckler left a comment

Choose a reason for hiding this comment

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

Great, thanks for implementing these changes.

Happy to merge it.

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.

Thanks for adding this!

The test contain some repetitions and can be simplified with fixtures, but let's move this to an issue.

@janfb janfb merged commit bb35def into main Apr 2, 2024
3 checks passed
@janfb janfb deleted the 1017 branch April 2, 2024 11:52
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.

3 participants