Skip to content
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

Dirichlet multinomial (continued) #4373

Merged
merged 57 commits into from
Jan 16, 2021

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Dec 22, 2020

Update: @bsmith89 has been working hard and this PR is getting close to review point.

The current state of the TODO list is:

  • Implement logp and random methods
  • Rename alpha parameter to a everywhere (to be consistent with Dirichlet, even though alpha would probably have been better for both)
  • Pass pre-commit
  • Get good-enough coverage from unittests
    • Test logp of DirichletMultinomial and BetaBinomial match when there are only two categories.
    • Pass on all tests that were adopted from Multinomial (except for test_batch_dirichlet_multinomial whose logic is not directly transferable)
    • Addapt test_batch_dirichlet_multinomial.
    • Test repr_latex
    • Test shaperror in .random()
    • Update docstrings to correctly reflect shape polymorphism (or at least the level of polymorphism that we explicitly test) (?). Update: The distribution is now working as the Multinomial. Future changes in this respect can be done in a separate PR that addresses both distributions.
  • Check issue with .random() failing when shape is not explicitly specified. Update: shape will be always required to avoid issues.
  • Mode algorithm is imperfect. Using self._defaultval instead to avoid confusing users.
  • Example notebook (see Dirichlet mixture of multinomials example pymc-examples#18)
  • Examine test coverage issues with conditional branches in random and logp methods (?)
  • Release note

@AlexAndorra
Copy link
Contributor

Awesome, thanks for picking that up @ricardoV94 ! Let me know when I can review 😉

@ricardoV94
Copy link
Member Author

Sure @AlexAndorra, but it might still take a while ;)

@AlexAndorra
Copy link
Contributor

Take your time, I'll be there -- I can't go outside anyway 😜

@bsmith89
Copy link
Contributor

@ricardoV94 I agree with all your skepticism of my initial implementation's shape management and tests. My intuition is that the API (and probably tests) should mirror Multinomial, but there is a lot going on there, so it'll be a bit of a slog.

Hopefully I'll be able to contribute to this PR. :)

@ricardoV94
Copy link
Member Author

@bsmith89 It would be great to have your input, specially since you started everything! Is there a way we could chat?

@bsmith89
Copy link
Contributor

For sure! I started a topic on the Discourse, so we don't have to spam this PR too much. Also happy to find a more synchronous way to discuss if that's helpful.

@AlexAndorra AlexAndorra added this to the vNext (3.11.0) milestone Dec 29, 2020
@twiecki twiecki marked this pull request as draft December 31, 2020 11:50
@twiecki twiecki changed the title WIP: Dirichlet multinomial (continued) Dirichlet multinomial (continued) Dec 31, 2020
@twiecki
Copy link
Member

twiecki commented Dec 31, 2020

Any progress on this?

@ricardoV94
Copy link
Member Author

@bsmith89 is working hard on this and I am helping with the unittests. We are waiting until we have some more progress before pushing new changes.

@twiecki
Copy link
Member

twiecki commented Dec 31, 2020

@ricardoV94 Indeed, just checked the thread and love the deep thinking and discussions that go into this!

@bsmith89
Copy link
Contributor

bsmith89 commented Jan 3, 2021

Ported over the Multinomial tests and got most of them passing (and I'll push those commits to this PR momentarily) but right now I'm having issues with a DM version of the Multinomial test tests.test_distributions.test_batch_multinomial.

@lucianopaz, I think you wrote that test in #4169. Can you explain what it's supposed to check? Could we get the same assurances with something more analogous to test_multinomial_vec_1d_n_2d_p (presumably something like test_multinomial_vec_2d_n_3d_p) which I'm finding easier to read and understand?

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jan 3, 2021

Ported over the Multinomial tests and got most of them passing (and I'll push those commits to this PR momentarily) but right now I'm having issues with a DM version of the Multinomial test tests.test_distributions.test_batch_multinomial.

@lucianopaz will certainly know better, but from a quick glance it seems to test that Multinomial batches (3d shape?) are correctly evaluating the logp and generating samples by initializing the distribution with sparse probability vectors where only one value/ category has a probability of 1 and everything else is 0. This makes it a deterministic function and hence easy to test, but it wouldn't work for the DM, since the Dirichlet component is never deterministic (and, more prosaically, because the alpha cannot be zero).

I might also be completely wrong :)

Are you sure you are passing valid alpha parameters to the DM copy of the test (i.e., not including any zero)? That should cause it to fail immediately. Whether fixing that would be enough to make it pass is another question :b

Also are you failing in the logp assertion, the random sample or both?

@lucianopaz
Copy link
Contributor

@bsmith89, what @ricardoV94 said was right. test_batch_multinomial was written to test that the Multinomial distribution didn't squeeze an n parameter that had more than 2 dimensions. It tests that the logp and the samples drawn from its random method match what is expected. This might not be needed in your case, as long as you don't squeeze your parameters (or reshape them in some hidden ways inside the distribution's __init__ or logp)

@bsmith89
Copy link
Contributor

bsmith89 commented Jan 3, 2021

Checking in on that now.

I'm on this PR branch (not my own) running pytest --pdb -k test_batch_dirichlet_multinomial pymc3/tests if that's helpful. By which I mean, the test I'm having issues with has been pushed here.

Comment on lines 727 to 729
if len(self.shape) > 1:
self.n = tt.shape_padright(n)
self.alpha = tt.as_tensor_variable(alpha) if alpha.ndim > 1 else tt.shape_padleft(alpha)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucianopaz @ricardoV94, not sure if this is quite what you meant, but I do the exact same reshaping as in the Multinomial.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I meant that in the Multinomial Distribution, we used to have a line in __init__ that did: n = np.squeeze(n). That line caused all sorts of problems when we had n.ndim > 1. This situation wasn't covered by existing tests so I wrote up test_batch_multinomial. A better approach would be to either parametrize a single test with multiple n and alpha values that span many shapes, or add test fixtures for n and alpha to achieve the same result.

@bsmith89
Copy link
Contributor

bsmith89 commented Jan 3, 2021

Gotta sign off for a bit, but I still haven't fully grokked what needs to happen with this test. Suggestions welcome! I'll take a look probably later today.

@ricardoV94
Copy link
Member Author

Using to_tuple function will be a good idea to handle these corner cases in random method. Something like -

Thanks, that seems to have solved it!

@ricardoV94
Copy link
Member Author

@bsmith89 Anything missing in this PR (not including the example Notebook)?

@bsmith89
Copy link
Contributor

bsmith89 commented Jan 12, 2021

LGTM!

Should I rebase onto main and add a release note, or are those last items someone else's responsibility?

@ricardoV94
Copy link
Member Author

Should I rebase onto main and add a release note, or are those last items someone else's responsibility?

Feel free to add the release note. It should work fine without rebasing. In the meanwhile I will open the PR for review

@ricardoV94 ricardoV94 marked this pull request as ready for review January 12, 2021 17:49
@ricardoV94
Copy link
Member Author

ricardoV94 commented Jan 12, 2021

@AlexAndorra If you are still interested in reviewing this one, we are ready :)

@bsmith89
Copy link
Contributor

@ricardoV94 not sure if I just messed up... 😕

There was conflict between my one line RELEASE-NOTES change and pymc3/master, so I followed GitHub's prompt to resolve that. Didn't realize it would merge everything else too...

Is that a problem?

@ricardoV94
Copy link
Member Author

@ricardoV94 not sure if I just messed up... confused

There was conflict between my one line RELEASE-NOTES change and pymc3/master, so I followed GitHub's prompt to resolve that. Didn't realize it would merge everything else too...

Is that a problem?

I think it's fine. It's still only showing 5 files changed

@AlexAndorra
Copy link
Contributor

Awesome @ricardoV94 and @bsmith89 ! I'll take some time to review this week 🥳
As this is a big PR, other reviews are welcome 😉

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done @ricardoV94 and @bsmith89, I really love that 😍
I just had a few comments below, mainly slight improvements that should be quick to implement if they are appropriate.

There are also two failing tests: one in MvNormal's sample_prior_predictive, so probably not related to this PR (cc @Sayam753); another in test_interval (this one is more mysterious to me -- does it ring any bells?)

pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved
pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved
pymc3/distributions/multivariate.py Show resolved Hide resolved
pymc3/distributions/multivariate.py Show resolved Hide resolved
pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved
@Sayam753
Copy link
Member

Thanks @AlexAndorra for the ping. The failing MvNormal test is a flaky test, reported in #4323 . So, rerunning the test suite can help. The second failing test case is not familiar to me as well.
Btw, great job @ricardoV94 @bsmith89 in putting this all together. I will also review the PR later today :)

@bsmith89
Copy link
Contributor

another in test_interval (this one is more mysterious to me -- does it ring any bells?)

I re-ran that test locally and it passed without problems. Seems like it might be a rare failing. Don't know why it would be stochastic...

Copy link
Member

@Sayam753 Sayam753 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor nitpicks below -

pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved
pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved
pymc3/distributions/multivariate.py Show resolved Hide resolved
pymc3/distributions/multivariate.py Show resolved Hide resolved
pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved
pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved
pymc3/distributions/multivariate.py Show resolved Hide resolved
Comment on lines +730 to +731

super().__init__(shape=shape, defaults=("_defaultval",), *args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dirichlet distribution makes use of get_test_value function to compute its distribution shape. Can we use get_test_value to determine shape here as well? Doing so, will even us help in #4379.

Ping @brandonwillard to ask how does get_test_value function work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is as simple, since the shape can be influenced by the n parameter as well as the a, whereas in the Dirichlet all information is necessarily contained in the a (when shape is not specified)

Copy link
Member Author

@ricardoV94 ricardoV94 Jan 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the Dirichlet functionality is wrapped in a DeprecationWarning (even though I don't seem to be able to trigger it), which suggests that they wanted to abandon that approach at some point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ricardoV94 , just a follow up, it indeed makes sense to avoid the use of get_test_value function as also discussed here #4000 (comment)

@bsmith89
Copy link
Contributor

@AlexAndorra, I think we've addressed your comments now.

  • The two test failures you pointed out appear to be unrelated to our patch
  • I'll submit a followup issue to update the docstring with an explanation for the random(... point=...) kwarg for contributors

Was there anything else?

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it looks all great to me now @bsmith89 😍 I'll wait for @Sayam753 to review before merging this great PR

@bsmith89
Copy link
Contributor

Oh! I thought they reviewed already. I see this indicator higher up in this thread:

image

Did you want him to do a second review given @ricardoV94's two commits since then?

@Sayam753
Copy link
Member

I'll submit a followup issue to update the docstring with an explanation for the random(... point=...) kwarg for contributors

I have seen point parameter being used for the likelihood distribution, either for computing log-likelihood or while doing posterior predictive sampling.

Copy link
Member

@Sayam753 Sayam753 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great work @ricardoV94 @bsmith89 🤩 . Last, there needs to be a mention in api source multivariate.rst for PyMC3 docs.

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all good now 🤩 Well done guys for this big PR 👏

@AlexAndorra AlexAndorra merged commit 2a3d9a3 into pymc-devs:master Jan 16, 2021
@ricardoV94 ricardoV94 deleted the dirichlet_multinomial_fork branch January 19, 2021 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants