Better arviz support in base inference #1269
Conversation
facebook-github-bot
added
the
CLA Signed
|
:D Ah, this is a nice feature to have for our next release. Thanks for working on this, Rob! I'm going to import the PR now just so that we can see the test results. I'll have to review this a bit later, though, since there're a few other pre-launch items that we have to focus on at the moment. In the mean time, there's a |
|
@horizon-blue has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
I think |
zaxtax
force-pushed
the
ot-better-arviz-support
branch
from
91deeee
to
4be80fc
Compare
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
17 similar comments
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
11 similar comments
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax Was facebook-github-bot acting crazy? Or did you actually update this PR 42 times this afternoon? 🤣 |
|
I force-pushed once and I think I upset the bot. I apologise to everyone
else you got that flood of emails 😅
…On Wed, 15 Dec 2021, 05:01 Xiaoyan Wang, ***@***.***> wrote:
@zaxtax <https://github.com/zaxtax> Was facebook-github-bot acting crazy?
Or did you actually update this PR 42 times this afternoon? 🤣
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1269 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCUPHGBOSTJRPIM2667TURAOI5ANCNFSM5KB2AA5A>
.
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>.
|
|
Haha don't worry, this is actually hilarious. I'm going to report this bug to our internal support team :) |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| new_mcs = MonteCarloSamples( | ||
| chain_results=samples, | ||
| num_adaptive_samples=self.num_adaptive_samples, | ||
| logll_results=logll, |
There was a problem hiding this comment.
Our internal linter (Pyre) is complaining about the following typing issue:
Incompatible parameter type [6]: Expected
Optional[List[Dict[RVIdentifier, torch.Tensor]]]for 3rd parameterlogll_resultsto callMonteCarloSamples.__init__but gotOptional[Dict[typing.Any, typing.Any]].
Speaking of which, do you mind adding a call to get_chain to the unit test as well? Thanks :D
There was a problem hiding this comment.
That should fix the linter error.
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@zaxtax has updated the pull request. You must reimport the pull request before landing. |
|
@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
I thought we still have a type error on this diff but seems like I was wrong :). Our internal tool does warn us that this PR is a bit old, so I'm going to rebase and rerun the internal tests. If the signal is still green then we are good to go. Thanks again for working on this, Rob! |
Motivation
When I was doing bug fixing I noticed my arviz changes were lost in the shuffle. These commits add the functionality while keeping the APIs unchanged as much as possible.
Changes proposed
Changes the
Test Plan
Please provide clear instructions on how the changes were verified. Attach screenshots if applicable.
Types of changes
Checklist