-
Notifications
You must be signed in to change notification settings - Fork 155
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
allow more flexible proposals in atomic snpe-c #732
Conversation
Codecov Report
@@ Coverage Diff @@
## main #732 +/- ##
=======================================
Coverage 74.25% 74.25%
=======================================
Files 79 79
Lines 6033 6033
=======================================
Hits 4480 4480
Misses 1553 1553
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -161,7 +161,8 @@ def train( | |||
# last proposal. | |||
proposal = self._proposal_roundwise[-1] | |||
self.use_non_atomic_loss = ( | |||
isinstance(proposal.posterior_estimator._distribution, mdn) | |||
isinstance(proposal, DirectPosterior) |
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.
how does this check change the behaviour for MCMCPosterior
or MultivariateNormal
?
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.
previously, the first check was isinstance(proposal.posterior_estimator._distribution, mdn)
. This raised an error if, e.g., proposal
was an MCMCPosterior
.
Now, the first check is isinstance(proposal, DirectPosterior)
which will be False
if proposal
is, e.g., an MCMCPosterior
. Because everything below is an and
case, these lines will never be executed.
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.
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.
the Python behaviour is clear to me, I was wondering about the sbi
semantic. But true, we want the line to be False
for MCMCPosterior
and other, instead of throwing an error. so all good!
@@ -161,7 +161,8 @@ def train( | |||
# last proposal. | |||
proposal = self._proposal_roundwise[-1] | |||
self.use_non_atomic_loss = ( | |||
isinstance(proposal.posterior_estimator._distribution, mdn) | |||
isinstance(proposal, DirectPosterior) |
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.
the Python behaviour is clear to me, I was wondering about the sbi
semantic. But true, we want the line to be False
for MCMCPosterior
and other, instead of throwing an error. so all good!
if one used, e.g., an
MCMCPosterior
, or simply atorch.distributions.MultivariateNormal
, one would get an error that the propsals has no attributeposterior_estimator
. This is fixed here.Resolves #706