-
Notifications
You must be signed in to change notification settings - Fork 346
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
Bug fix in pyromixin for posterior sampling #1158
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1158 +/- ##
==========================================
+ Coverage 90.27% 90.74% +0.46%
==========================================
Files 93 95 +2
Lines 7341 7668 +327
==========================================
+ Hits 6627 6958 +331
+ Misses 714 710 -4
Continue to review full report at Codecov.
|
@adamgayoso please have a look at this |
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.
LGTM outside of the small request for docstring
scvi/model/base/_pyromixin.py
Outdated
@@ -262,7 +262,7 @@ def _get_obs_plate_return_sites(self, return_sites, obs_plate_sites): | |||
else: | |||
return obs_plate_sites | |||
|
|||
def _get_obs_plate_sites(self, args, kwargs): | |||
def _get_obs_plate_sites(self, args, kwargs, sample_observed): |
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.
can you document this function and add relevant typing?
I think this is done now. |
Yes, you are right actually - these arguments are not optional and don't
have default values. I will change this.
…On Wed, 22 Sep 2021, 05:56 Adam Gayoso, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In scvi/model/base/_pyromixin.py
<#1158 (comment)>:
> + args: Optional[list],
+ kwargs: Optional[dict],
usually optional has a None default. Don't believe this is the case here?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1158 (review)>,
or unsubscribe
<https:/notifications/unsubscribe-auth/AFMFTVYDBGGSSQDKZ5B3L6TUDFOXJANCNFSM5D663UCQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
The method
._get_obs_plate_sites()
should discard observed variables (unless requested otherwise) and plates. Current code always performs posterior sampling even if unobserved local variables do not exist in the model, e.i. code never enters this statement:This PR addresses the issue by adding a check for observed and plates.