Better arviz support in base inference #1269

zaxtax · 2021-12-14T20:10:46Z

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

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
The title of my pull request is a short description of the requested changes.

horizon-blue · 2021-12-14T21:57:05Z

: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 get_chain method that returns another MonteCarloSamples that people sometimes use to retrieve the data for a particular chain. Could you check if that's still working as expected with your change?

facebook-github-bot · 2021-12-14T22:07:09Z

@horizon-blue has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zaxtax · 2021-12-14T22:07:51Z

I think get_chain should still work like it did before, but I should pass likelihoods to the MonteCarloSamples object returned by that method.

facebook-github-bot · 2021-12-15T00:50:22Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:50:25Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:51:16Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:51:19Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:52:12Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:52:15Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:52:56Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:52:58Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:53:39Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:53:41Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:54:35Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:54:37Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:55:43Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:55:46Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:56:24Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:56:27Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:57:04Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T00:57:06Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:03:31Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:03:34Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:04:21Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:04:24Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:04:58Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:05:01Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:05:52Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:05:55Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:06:57Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:07:00Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:08:45Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T01:08:48Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

horizon-blue · 2021-12-15T05:00:52Z

@zaxtax Was facebook-github-bot acting crazy? Or did you actually update this PR 42 times this afternoon? 🤣

zaxtax · 2021-12-15T05:05:43Z

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>.

horizon-blue · 2021-12-15T05:14:16Z

Haha don't worry, this is actually hilarious. I'm going to report this bug to our internal support team :)

facebook-github-bot · 2021-12-15T17:57:43Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-15T21:54:25Z

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

horizon-blue · 2021-12-16T06:32:05Z

src/beanmachine/ppl/inference/monte_carlo_samples.py

+        new_mcs = MonteCarloSamples(
+            chain_results=samples,
+            num_adaptive_samples=self.num_adaptive_samples,
+            logll_results=logll,


Our internal linter (Pyre) is complaining about the following typing issue:

Incompatible parameter type [6]: Expected Optional[List[Dict[RVIdentifier, torch.Tensor]]] for 3rd parameter logll_results to call MonteCarloSamples.__init__ but got Optional[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

That should fix the linter error.

facebook-github-bot · 2021-12-16T07:50:05Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-16T10:14:38Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-16T10:27:07Z

@zaxtax has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-12-20T18:38:33Z

@horizon-blue has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

horizon-blue · 2022-01-06T19:48:20Z

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!

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 14, 2021

zaxtax requested a review from horizon-blue December 14, 2021 20:18

zaxtax added 6 commits December 14, 2021 23:00

Adding log-likelhood calculations into base inference

5b2f449

Format fix

84cd35c

Test for log_likelihood fields in MonteCarloSamples

889ab9e

Format fix

cb03a5b

Fix None case

2d15c6c

Update get_chain to include log-likelihoods

4be80fc

zaxtax force-pushed the ot-better-arviz-support branch from 91deeee to 4be80fc Compare December 14, 2021 23:00

Reorder MonteCarloSamples arguments

c8d2d46

horizon-blue reviewed Dec 16, 2021

View reviewed changes

Fix linter errors and add get_chain test

b63f36b

Add more unit tests

8f87f5d

Use MonteCarloSamples API more

ebea5a0

facebook-github-bot closed this in d0bc53a Jan 7, 2022

horizon-blue deleted the ot-better-arviz-support branch February 3, 2022 04:26

Better arviz support in base inference #1269

Better arviz support in base inference #1269

Conversation

zaxtax commented Dec 14, 2021 • edited by michaeltingley Loading

Motivation

Changes proposed

Test Plan

Types of changes

Checklist

horizon-blue commented Dec 14, 2021

facebook-github-bot commented Dec 14, 2021

zaxtax commented Dec 14, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

horizon-blue commented Dec 15, 2021

zaxtax commented Dec 15, 2021 via email

horizon-blue commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

facebook-github-bot commented Dec 15, 2021

horizon-blue Dec 16, 2021

Choose a reason for hiding this comment

zaxtax Dec 16, 2021

Choose a reason for hiding this comment

facebook-github-bot commented Dec 16, 2021

facebook-github-bot commented Dec 16, 2021

facebook-github-bot commented Dec 16, 2021

facebook-github-bot commented Dec 20, 2021

horizon-blue commented Jan 6, 2022

zaxtax commented Dec 14, 2021 •

edited by michaeltingley

Loading