- Notifications
You must be signed in to change notification settings - Fork180
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
fix: add NeuralPosteriorEnsemble to utils.__init__#1002
fix: add NeuralPosteriorEnsemble to utils.__init__#1002
Conversation
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 tackling this@jnsbck! Currently tests fail as there is a circular import error. I suspect the reason is becausesbi.utils.posterior_ensemble imports fromsbi.inference, which can cause this issue. I would suggest movingposterior_ensemble underinference.posteriors. Does this seem reasonable to you?
@gmoss13 thanks for having a look. I figured out that the circular dependency is introduced by isort! Not running it means the diff reduces to one line, as I had originally proposed as the fix for this. |
bb80dd7 toc324c55CompareCodecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #1002 +/- ##==========================================- Coverage 76.37% 76.21% -0.17%========================================== Files 84 85 +1 Lines 6507 6524 +17 ==========================================+ Hits 4970 4972 +2- Misses 1537 1552 +15
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. |
Renamed All checks are passing, but codecov fails for some reason. |
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@jnsbck! Great to see the tests pass. I am not too worried about the codecov alerts onsbi.inference.posteriors.ensemble_posterior as the code was not changed from what it was onutils, so any of these lines would not have been covered before anyway. Before merging, I would add a small API test to make sure we can still importNeuralPosteriorEnsemble andEnsemblePotential from utils.
| device: str = "cpu", | ||
| ): | ||
| warnings.warn( | ||
| "EnsemblePotential was moved to sbi.inference.posteriors.ensemble_posterior. \ |
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.
great!
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.
tested this locally, but will add tests :)
Should be good to merge if tests pass :) |
…ls' of github.com:sbi-dev/sbi into 744-allow-import-of-neuralposteriorensemble-from-sbiutils
Great work! |
What does this implement/fix? Explain your changes
Rename
NeuralPosteriorEnsembletoEnsemblePosteriorand moveutils.posterior_ensembletoinference.posteriors.ensemble_posteriorsDoes this close any currently open issues?
Fixes#744
Any relevant code examples, logs, error output, etc?
nope
Any other comments?
nope
Checklist
Put an
xin the boxes that apply. You can also fill these out after creatingthe 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.
guidelines
with
pytest.mark.slow.guidelines
main(or there are no conflicts withmain)