Port Dirichlet Multinomial to v4 #4758

AlexAndorra · 2021-06-08T11:19:35Z

Create a new RV op by inheriting from aesara RandomVariable, porting the code in the old random method to the rng_fn classmethod of the new RandomVariable.
Add the new (instance) of the RandomVariable to the rv_op field in the respective PyMC3 distribution.
Port any initialization logic in the old __init__ to the new dist classmethod
Refactor logp and logcdf to expect arguments in this order: value, arg1, arg2, ... argn
Re-enable tests in test_distributions.py
Create new test in test_distributions_random.py and remove old BaseTestCase in that file.
Re-enable other tests that depended on this distribution in other test files.
Run linting / style checks

pymc3/distributions/multivariate.py

ricardoV94

Thanks for opening up the PR! I left some comments in case it helps figuring things out. Multivariate distributions are a bit of PITA :/

pymc3/distributions/multivariate.py

pymc3/tests/test_distributions.py

AlexAndorra · 2021-06-30T17:10:35Z

This is ready for review and merge @ricardoV94 -- thanks a lot for your help 🍾
The rebase on main was actually a very bad idea -- that picked up a lot of commits and changes that are not mine; sorry about that 🤦‍♂️ I'll merge main for the next PR...

ricardoV94 · 2021-06-30T18:37:40Z

This is ready for review and merge @ricardoV94 -- thanks a lot for your help champagne
The rebase on main was actually a very bad idea -- that picked up a lot of commits and changes that are not mine; sorry about that man_facepalming I'll merge main for the next PR...

I think you will have to make another PR or clean the git history to fix the commit issues

michaelosthege · 2021-06-30T21:41:51Z

Looks like you rebased with some weird settings that preserved the chronological order of commits?
I hope you can copy/paste your changes onto a new branch?

brandonwillard · 2021-06-30T22:20:13Z

The rebase on main was actually a very bad idea -- that picked up a lot of commits and changes that are not mine; sorry about that I'll merge main for the next PR...

It looks like this branch wasn't rebased onto main; that's the problem.

Also, there's no need to create a new branch/PR, especially since that could easily involve much more work. If you rebase onto main from the branch's current state, it will stop at the commits that conflict and you can decide whether or not to keep them. Given the duplicate commits in this latter part of this branch, it looks like you can check out the changes labeled "ours" and drop the most recent duplicate commits.

brandonwillard · 2021-06-30T22:24:04Z

I just performed the rebase locally and checked out only "our" changes on each conflict; the result has an empty diff when compared with this upstream branch (i.e. the aforementioned rebase onto main leaves the code in the same state it's currently in, while also fixing the bad history).

I can push this rebase if you'd like.

AlexAndorra · 2021-07-01T09:44:24Z

Ah yes that'd be great, thanks @brandonwillard ! Tell me if I need to do anything else

AlexAndorra · 2021-07-02T08:04:01Z

Magic 🧙‍♂️ Thanks @brandonwillard !
I also fixed a last failing test (test_batch_dirichlet_multinomial) so this should really be ready for review and merge now @ricardoV94 🍾

pymc3/distributions/multivariate.py

pymc3/tests/test_distributions.py

ricardoV94 · 2021-07-02T08:22:57Z

Your pre-commit seems to have changed a lot of lines that were not related to the PR, which makes it difficult to review the changes.

Having an explicit comma after the last item in a multi-line call / tuple prevents it from auto-collapsing everything into a single line.

AlexAndorra · 2021-07-03T17:39:38Z

WHy do you want to prevent it from being on a single line if it's passing black?

michaelosthege · 2021-07-03T17:50:11Z

WHy do you want to prevent it from being on a single line if it's passing black?

These lines are likely to cause git conflicts, because the same files are edited in other PRs as well. I don't understand why this happened in the first place though.

Apart from that (and the git conflict which looks reasonably easy to fix) how far away from the finish line is this PR ?

ricardoV94 · 2021-07-04T04:59:58Z

WHy do you want to prevent it from being on a single line if it's passing black?

Besides what @michaelosthege said, it's an issue that this was an accidental change. The next person could come around and introduce the multiline fomat again (which also passes black) and we would go back and forth over time without need, just complicating reviews and the git history.

For example most of the same tests in those files keep the multiline format. If we want one style over the other it should be enforced automatically (and consistently).

Also, I think these multilines are easier to read than the collapsed format, since most don't use kwargs.

AlexAndorra · 2021-07-04T15:18:12Z

I don't understand why this happened in the first place though.

Yeah me neither, I didn't change anything manually -- formatting was done automatically by pre-commit locally.

Apart from that, how far away from the finish line is this PR ?

It's done @michaelosthege. Once tests pass and conflicts are resolved, it can be merged.

Also, I think these multilines are easier to read than the collapsed format, since most don't use kwargs.

I agree with all that. I just don't know how these changes appeared in the first place. If they were introduced by pre-commit, it seems to me there is a low chance they'll be changed again in a further PR.

ricardoV94 · 2021-07-04T15:37:06Z

I agree with all that. I just don't know how these changes appeared in the first place. If they were introduced by pre-commit, it seems to me there is a low chance they'll be changed again in a further PR.

I don't know either. But looking at the black github, many issues seem related to this type of trailing commas...

codecov · 2021-07-04T15:55:55Z

Codecov Report

Merging #4758 (5569839) into main (125256f) will decrease coverage by 10.34%.
The diff coverage is 100.00%.

❗ Current head 5569839 differs from pull request most recent head 532d869. Consider uploading reports for the commit 532d869 to get more accurate results

@@             Coverage Diff             @@
##             main    #4758       +/-   ##
===========================================
- Coverage   71.97%   61.63%   -10.35%     
===========================================
  Files          85       85               
  Lines       13839    13843        +4     
===========================================
- Hits         9961     8532     -1429     
- Misses       3878     5311     +1433

Impacted Files	Coverage Δ
pymc3/distributions/multivariate.py	`62.10% <100.00%> (+5.22%)`	⬆️
pymc3/ode/utils.py	`17.85% <0.00%> (-82.15%)`	⬇️
pymc3/step_methods/mlda.py	`11.94% <0.00%> (-74.55%)`	⬇️
pymc3/gp/cov.py	`29.20% <0.00%> (-68.88%)`	⬇️
pymc3/ode/ode.py	`24.24% <0.00%> (-60.61%)`	⬇️
pymc3/step_methods/hmc/hmc.py	`33.33% <0.00%> (-58.83%)`	⬇️
pymc3/step_methods/metropolis.py	`35.49% <0.00%> (-47.77%)`	⬇️
pymc3/tests/models.py	`40.28% <0.00%> (-47.49%)`	⬇️
pymc3/gp/mean.py	`51.28% <0.00%> (-46.16%)`	⬇️
pymc3/gp/util.py	`38.02% <0.00%> (-42.26%)`	⬇️
... and 24 more

ricardoV94 · 2021-07-05T08:33:29Z

I cleaned up the history and force-pushed here. Checking if the tests pass or if I screwed up something

AlexAndorra · 2021-07-05T10:51:47Z

Wasn't the history already cleaned by Brandon?
In any case, all tests are green now, so feel free to review and merge 🥳

ricardoV94 · 2021-07-05T11:23:03Z

Wasn't the history already cleaned by Brandon?

I reverted the accidental formatting issues

ricardoV94 · 2021-07-05T11:23:26Z

Thanks @AlexAndorra this one was tough! :)

AlexAndorra added the v4 label Jun 8, 2021

AlexAndorra added this to the vNext (4.0.0) milestone Jun 8, 2021

AlexAndorra self-assigned this Jun 8, 2021

AlexAndorra mentioned this pull request Jun 8, 2021

Port remaining distributions to v4 #4686

Closed

26 tasks

ricardoV94 reviewed Jun 8, 2021

View reviewed changes

pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Jun 8, 2021

View reviewed changes

pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Jun 8, 2021

View reviewed changes

AlexAndorra commented Jun 8, 2021

View reviewed changes

pymc3/distributions/multivariate.py Show resolved Hide resolved

AlexAndorra commented Jun 8, 2021

View reviewed changes

pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Jun 8, 2021

View reviewed changes

pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved

AlexAndorra commented Jun 9, 2021

View reviewed changes

pymc3/tests/test_distributions.py Outdated Show resolved Hide resolved

AlexAndorra commented Jun 11, 2021

View reviewed changes

pymc3/tests/test_distributions.py Show resolved Hide resolved

AlexAndorra marked this pull request as ready for review June 30, 2021 17:05

AlexAndorra requested a review from ricardoV94 June 30, 2021 17:05

brandonwillard force-pushed the port-dirichlet-mn branch from 97fa5a6 to dd608fc Compare July 1, 2021 18:23

ricardoV94 reviewed Jul 2, 2021

View reviewed changes

pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved

ricardoV94 reviewed Jul 2, 2021

View reviewed changes

pymc3/tests/test_distributions.py Outdated Show resolved Hide resolved

AlexAndorra added 2 commits July 5, 2021 10:31

Refactor DirichletMultinomial for V4

3cd9cb6

Added test_multinomial_zero_probs test

532d869

ricardoV94 force-pushed the port-dirichlet-mn branch from 5569839 to 532d869 Compare July 5, 2021 08:32

AlexAndorra requested a review from ricardoV94 July 5, 2021 10:52

michaelosthege approved these changes Jul 5, 2021

View reviewed changes

ricardoV94 merged commit 223b0b8 into main Jul 5, 2021

michaelosthege deleted the port-dirichlet-mn branch July 5, 2021 11:25

ricardoV94 mentioned this pull request Jul 5, 2021

Test multinomial distribution with probability vector containing zeros #4487

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port Dirichlet Multinomial to v4 #4758

Port Dirichlet Multinomial to v4 #4758

AlexAndorra commented Jun 8, 2021 •

edited

Loading

ricardoV94 left a comment •

edited

Loading

AlexAndorra commented Jun 30, 2021 •

edited

Loading

ricardoV94 commented Jun 30, 2021

michaelosthege commented Jun 30, 2021

brandonwillard commented Jun 30, 2021

brandonwillard commented Jun 30, 2021

AlexAndorra commented Jul 1, 2021 •

edited

Loading

AlexAndorra commented Jul 2, 2021

ricardoV94 commented Jul 2, 2021 •

edited

Loading

AlexAndorra commented Jul 3, 2021

michaelosthege commented Jul 3, 2021

ricardoV94 commented Jul 4, 2021 •

edited

Loading

AlexAndorra commented Jul 4, 2021

ricardoV94 commented Jul 4, 2021

codecov bot commented Jul 4, 2021 •

edited

Loading

ricardoV94 commented Jul 5, 2021 •

edited

Loading

AlexAndorra commented Jul 5, 2021

ricardoV94 commented Jul 5, 2021

ricardoV94 commented Jul 5, 2021

Port Dirichlet Multinomial to v4 #4758

Port Dirichlet Multinomial to v4 #4758

Conversation

AlexAndorra commented Jun 8, 2021 • edited Loading

ricardoV94 left a comment • edited Loading

Choose a reason for hiding this comment

AlexAndorra commented Jun 30, 2021 • edited Loading

ricardoV94 commented Jun 30, 2021

michaelosthege commented Jun 30, 2021

brandonwillard commented Jun 30, 2021

brandonwillard commented Jun 30, 2021

AlexAndorra commented Jul 1, 2021 • edited Loading

AlexAndorra commented Jul 2, 2021

ricardoV94 commented Jul 2, 2021 • edited Loading

AlexAndorra commented Jul 3, 2021

michaelosthege commented Jul 3, 2021

ricardoV94 commented Jul 4, 2021 • edited Loading

AlexAndorra commented Jul 4, 2021

ricardoV94 commented Jul 4, 2021

codecov bot commented Jul 4, 2021 • edited Loading

Codecov Report

ricardoV94 commented Jul 5, 2021 • edited Loading

AlexAndorra commented Jul 5, 2021

ricardoV94 commented Jul 5, 2021

ricardoV94 commented Jul 5, 2021

AlexAndorra commented Jun 8, 2021 •

edited

Loading

ricardoV94 left a comment •

edited

Loading

AlexAndorra commented Jun 30, 2021 •

edited

Loading

AlexAndorra commented Jul 1, 2021 •

edited

Loading

ricardoV94 commented Jul 2, 2021 •

edited

Loading

ricardoV94 commented Jul 4, 2021 •

edited

Loading

codecov bot commented Jul 4, 2021 •

edited

Loading

ricardoV94 commented Jul 5, 2021 •

edited

Loading